Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Add warning for AgMIP probabilty distributions and return nothing #30

Merged
merged 2 commits into from
Apr 23, 2024

Conversation

lrennels
Copy link
Collaborator

@lrennels lrennels commented Apr 23, 2024

Issue: The get_probdists_gtap_df function does not take the damage function as an argument, so it returns samples from a probability distributionconstructed from a distribution of lowDF, midDF, and highDF for all damage function settings.

Update: The function will now return those samples only if the damage function is one of lowDF, midDF, and highDF, and otherwise return nothing since we do not currently have a method to produce distributions for that damage function. In addition, this PR adds the parameter gtap_name to the component so that any model using the component can easily recover the name of the damage function used to specific the component.

@djsmithumn and @bryanparthum for awareness

@lrennels lrennels requested a review from davidanthoff April 23, 2024 02:11
@codecov-commenter
Copy link

codecov-commenter commented Apr 23, 2024

Codecov Report

Attention: Patch coverage is 0% with 11 lines in your changes are missing coverage. Please review.

Project coverage is 78.46%. Comparing base (177e8e7) to head (76ed7e4).

Files Patch % Lines
src/core/mcs.jl 0.00% 11 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #30      +/-   ##
==========================================
- Coverage   82.25%   78.46%   -3.80%     
==========================================
  Files           6        6              
  Lines          62       65       +3     
==========================================
  Hits           51       51              
- Misses         11       14       +3     
Flag Coverage Δ
unittests 78.46% <0.00%> (-3.80%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@lrennels lrennels merged commit 053a84c into main Apr 23, 2024
20 checks passed
@lrennels lrennels deleted the agmip-mcs branch April 23, 2024 04:03
@bryanparthum
Copy link

bryanparthum commented Apr 23, 2024

Nice addition @lrennels! Will this alter the overall size of the mcs? i.e., will the random parameters of the other (non-ag) components receive different values for no other reason than changing the Ag damage function setting? Or does the passing of nothing maintain the ordering for other mcs parameter draws?

@lrennels
Copy link
Collaborator Author

lrennels commented Apr 23, 2024

@bryanparthum ah that's a good point, I assume your concern being if you wanted to apples to apples compare a Monte Carlo Simulation with agMIP or a non-agMIP and that in this case the component parameter draws would differ? You're right this would break that consistency, as you would have fewer calls ... one way I could fix that would be to make the function act the same each time but then just return either a Vector or nothing.

@bryanparthum
Copy link

bryanparthum commented Apr 23, 2024

You got it (assuming this is still sensitive to the n in the mcs). I think passing a vector of length n of nothings would be okay, no? But you obviously know what the most consistent way to code it would be. If I were (am?) a user looking at the mcs parameters, a vector of nothing for the Ag component would be intuitive.

@lrennels
Copy link
Collaborator Author

Really good catch, a vector of nothing would be fine but won't fix the issue, since that still wouldn't call rand. Instead see the commit I just added, which basically makes sure the get_prob_dists function behaves the same way re. sampling and then just chooses what to return.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants