-
Notifications
You must be signed in to change notification settings - Fork 2
Remove Default implementation for unit types
#1145
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -180,7 +180,10 @@ impl CommodityPrices { | |
| // NB: Time slice fractions will sum to one | ||
| let weight = time_slice_info.time_slices[time_slice_id] / Year(1.0); | ||
| let key = (commodity_id.clone(), region_id.clone()); | ||
| *weighted_prices.entry(key).or_default() += *price * weight; | ||
| weighted_prices | ||
| .entry(key) | ||
| .and_modify(|v| *v += *price * weight) | ||
| .or_insert_with(|| *price * weight); | ||
| } | ||
|
|
||
| weighted_prices | ||
|
|
@@ -352,7 +355,7 @@ where | |
| /// | ||
| /// Note: this should be similar to the "shadow price" strategy, which is also based on marginal | ||
| /// costs of the most expensive producer, but may be more successful in cases where there are | ||
| // multiple SED/SVD outputs per asset. | ||
| /// multiple SED/SVD outputs per asset. | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Surprised this sort of thing doesn't get flagged by any of the checks, to he honest
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah... it seems in general there aren't any tools that check doc comments, which is a bit annoying. |
||
| /// | ||
| /// # Arguments | ||
| /// | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This continues to treat a missing commodity price as zero via
unwrap_or(MoneyPerFlow(0.0)). Given the motivation for removingDefaulton unit types (to avoid silently inferring missing prices as free), consider changing this call path to propagate absence (e.g., returnOption/Resultor require callers to provide a fallback explicitly at the call site where it’s truly intended).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is beyond the scope of this PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you remember why we were treating missing prices as zero here? Are there cases where we might not have prices yet?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some commodities genuinely don't have a price (all OTH commodities are unpriced), so it's fair to treat them as free in this case. Other (non-OTH) commodities may have a missing price because there are no producers of that commodity and no potential producers of that commodity in the following year. In this case it's probably NOT a good idea to treat the commodities as free, and this may be responsible for some of the weird prices Adam is describing in #1130