Skip to content

Add quantity: CarbonIntensity (Mass/Energy)#410

Merged
iliekturtles merged 1 commit into
iliekturtles:masterfrom
robinohs:master
Apr 1, 2023
Merged

Add quantity: CarbonIntensity (Mass/Energy)#410
iliekturtles merged 1 commit into
iliekturtles:masterfrom
robinohs:master

Conversation

@robinohs

@robinohs robinohs commented Mar 24, 2023

Copy link
Copy Markdown
Contributor

During the development of code related to carbon intensity we did not find any quantity that can express mass per energy.
In this PR we propose a quantity we named CarbonIntensity.
A sample unit for this quantity would be g/kWh, which we use to express gCO2/kWh.
Maybe the name CarbonIntensity is already too specific and could be changed to something more general.
I would be happy to discuss about that.

@robinohs robinohs changed the title Add quantity: Carbon intensity Add quantity: Carbon intensity Mar 24, 2023
@robinohs robinohs changed the title Add quantity: Carbon intensity Add quantity: Carbon intensity Mar 24, 2023
@robinohs robinohs changed the title Add quantity: Carbon intensity Add quantity: CarbonIntensity Mar 24, 2023
@robinohs robinohs changed the title Add quantity: CarbonIntensity Add quantity: CarbonIntensity (Mass/Energy) Mar 24, 2023
@iliekturtles

Copy link
Copy Markdown
Owner

Thanks for the PR!

CarbonIntensity is too specific, but I'm not having any success finding what the quantity name should be. I'll keep searching, but perhaps MassPerEnergy is the best description. I'll also try to get my review of the changes done this weekend.

@robinohs

Copy link
Copy Markdown
Contributor Author

MassPerEnergy sounds good to me.
If you wish, I can do the desired name change in the PR after you fixed a name.

Thx for the quick response.

@iliekturtles

Copy link
Copy Markdown
Owner

I did find a reference to EmissionFactor which in turn lead me to emission intensity (aka carbon intensity). I think this is still too specific. Wait on name changes for the moment. I'll do some more searching this weekend to see if there is anything more appropriate.

Regardless of what the quantity ends up being named, emission intensity and carbon intensity should be prominently mentioned in the documentation as this appears to be the primary use for the quantity.

@iliekturtles

Copy link
Copy Markdown
Owner

Changes look good. I pushed a bunch of formatting fixes to your fork. Could you please review, go ahead with the change to MassPerEnergy, and squash everything down to a single commit?

@robinohs

Copy link
Copy Markdown
Contributor Author

I changed the name to MassPerEnergy and squashed all commits into a single one.

@iliekturtles iliekturtles left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couple more comments with this review. Could you also update the commit message:

Add mass per energy quantity.

Generalization of emission intensity (carbon intensity) as mass per energy.

Comment thread src/si/mod.rs Outdated
areal_number_rate::ArealNumberRate,
available_energy::AvailableEnergy,
capacitance::Capacitance,
mass_per_energy::MassPerEnergy,

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-sort mod.rs:

Suggested change
mass_per_energy::MassPerEnergy,
diff --git a/src/si/mod.rs b/src/si/mod.rs
index 5584c84..4d68ba3 100644
--- a/src/si/mod.rs
+++ b/src/si/mod.rs
@@ -61,7 +61,6 @@ system! {
         areal_number_rate::ArealNumberRate,
         available_energy::AvailableEnergy,
         capacitance::Capacitance,
-        mass_per_energy::MassPerEnergy,
         catalytic_activity::CatalyticActivity,
         catalytic_activity_concentration::CatalyticActivityConcentration,
         curvature::Curvature,
@@ -113,6 +112,7 @@ system! {
         mass_concentration::MassConcentration,
         mass_density::MassDensity,
         mass_flux::MassFlux,
+        mass_per_energy::MassPerEnergy,
         mass_rate::MassRate,
         molality::Molality,
         molar_concentration::MolarConcentration,

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reordering was done in the latest squashed commit.

Comment thread src/si/mass_per_energy.rs
@@ -0,0 +1,121 @@
//! Mass per energy (base unit kilogram per joule, m⁻² · s²).

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add something about mass per energy most commonly being used as emission intensity/carbon intensity.

@robinohs robinohs Mar 29, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a description of the typical use case of the generalization and a short explanation what is expressed by that use case.

@robinohs

robinohs commented Mar 29, 2023

Copy link
Copy Markdown
Contributor Author

I implemented the requested changes.

Generalization of emission intensity (carbon intensity) as mass per energy.
@iliekturtles iliekturtles merged commit c6603db into iliekturtles:master Apr 1, 2023
@iliekturtles

Copy link
Copy Markdown
Owner

I pushed some formatting for the module comment and merged. Thanks so much for the PR!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants