-
Notifications
You must be signed in to change notification settings - Fork 1
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 to support SpanInstabase in Labor #69
Conversation
My test for labor works, but... @brews I've lost the ability to run
I've tried to run pytest in different ways, but even if I get that line running, it messes up how other imports are interpreted. Any thoughts? |
@jrising This looks like Installing |
@brews Okay, that got me working again. Thanks. But my test failed with a segfault on the ag system test...
Can you run this on the cloud? |
@jrising Yeah, I can run the tests. I'll post back when they're done. Bummer that the CLI test also fail. From your virtual environment can you run |
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.
Cool!
I have some suggestions but I don't see anything breaking!
This PR has three related changes:
SpanInstabase
to be embedded within theFractionSum
calculation.RecursiveCalculation
ABC, to better ensure passing of messages to contained subcalculations.Reword
calculation, used to label the operations that need to be kept for labor.These are in response to https://gitlab.com/ClimateImpactLab/Impacts/impact-calculations/-/issues/141.
Strictly speaking, only the first item is necessary. However, only that change results in three different outputs named
rebased
,rebased2
, andrebased22
. We can clean up the calculation structure used in labor and keep only the results needed by allowing calculation results to be renamed in situ. That requires theReword
calculation, and it is at this point where I noticed that a lot of similar recursive calculations don't properly pass messages along, and so I madeReword
and several other calculations derive from the newRecursiveCalculation
for ease.