-
Notifications
You must be signed in to change notification settings - Fork 0
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
S1 aft treemaker #102
S1 aft treemaker #102
Conversation
does this mean all the current SR1 minitrees need to be regenerated? |
No, this makes a new minitree |
Thanks @darrylmasson , but would prefer to add it to our Extended minitree to avoid too many. Could you make a new PR there? |
I also took the liberty of bumping the Extended minitree version to 0.0.4 with this PR, which will require regrowing all the minitrees. Also worth pointing out that this PR won't work for pax < pax_v6.6.0. |
thanks @darrylmasson , looks good to me, will merge it together with other updates at a later stage. |
When trying to load my SR0 analysis I just found out that the 'Extended' minitrees have not been remade, which means that hax returns me an empty dataframe because I use make_minitrees=False. Because of the bumping of the Extended minitree version to 0.0.4 these also need to be remade in the common folder to keep our analysis code working. Who will do that? @feigaodm maybe check that for every minitree maker that gets bumbed the minitrees also are remade? (pax v 6.4.2) Thanks |
Next problem: When trying to remake the Extended minitrees myself I get: AttributeError: 'Interaction' object has no attribute 's1_area_fraction_top_probability' Of course this is expected as this is a new pax feature, and the SR0 analysis is done on 6.4.2 But this now seems to mean we cannot rerun any SR0 analysis with the pax version used in the paper. I think this is a problem as we should always be able to redo it. Another option would be for me to downgrade pax for a single analysis.... Any ideas how to fix this and how to handle this in the future? |
A horrible workaround is monkeypatching your minitree version:
Then you can load the old minitrees again. However, as you say, the real solution is remaking the minitrees automatically (or at least semi-automatically, i.e. someone has a script and does it) whenever a version bump occurs. The pax version is a bigger problem though: even if the trees are remade, pax 6.4.2 doesn't have this info. Probably a solution would be to have the minitree check the pax version of the file it's loading in, and don't try to add this column / activate these branches if it is too low. |
We can at least put that line into a try-except block, but the question is what we'd do in the < pax_v6.6.0 case. Option 1 is we set the value to |
I made a new PR #112 to fix this. |
@sanderbreur Thanks for pointing this out, I thought we are all going to use pax>6.6.2 for future analysis, especially something related to run-combinations. Can you try 6.6.2 instead to see if there is a problem still? |
Adds a treemaker to store the s1 area fraction top pvalue, so lax doesn't have to compute it at runtime every time.