-
Notifications
You must be signed in to change notification settings - Fork 138
Climate drivers for fire for REF #3975
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
base: main
Are you sure you want to change the base?
Conversation
… ref_fire_diagnostic
For the reviews?
|
we know @douglask3 very well - Dougie, accept the invite @axel-lauer has sent you today pls, bud 🍺 |
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 started the tech review - will head to the pub in a short bit, so I'll finish it on Monday 🍺
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.
OK: next stop is fire_diagnostic.py
, but I needs lunch
if len(parts) == 2: | ||
variable_name = parts[0].strip() | ||
variable_value = parts[1].strip() | ||
if callable(variable_value): |
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.
just out of curiosity, what file is that that contains both executable code and strings?
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.
The relevant file is this one esmvaltool/diag_scripts/fire/parameter_files/variables_info-6-frac_points_0.2-nvariables-frac_random_sample0.2-nvars_6-niterations_5000.txt.
The potential executable relates I suppose to this line for example in the variable file used here:
Line 7 in e5bb44c
subset_function:: sub_year_months |
I am not sure if in the context of the REF we can expect the particular case of callable functions in this file and thus could get rid of that? We would need @douglask3 to the rescue here to clarify that!
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.
Leaving these comments opened still for now before additional information.
SyntaxError, | ||
MemoryError, | ||
RecursionError, | ||
) as expt: |
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.
looks to me you really want to catch any exception here, so perhaps except Exception as expt:
is better
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.
btw why is the handling of that file so fragile that you may expect everyone and their dogs exceptions? Maybe a pre-formatting of it could help?
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 I replaced the except Exception as expt:
due to Codacy errors but indeed it is unclear what can be expected there in the file.
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.
Parameters | ||
---------- | ||
file_name: str | ||
The name of the file containing the variables. |
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.
having just seen how fragile the handling of that file may be, could you please add a few words about it here: file type, extension, and a few of its characteristics
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.
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.
OK done with the tech review myself - @jlenh please don't be put off by the number of change requests, they are mostly minor - and, if you think they may yield an unrealistic timescale for integration with the REF, pls ignore (for now) anything. A good idea to open an issue with the bits that we may want to address at a later stage, methinks 🍻
Co-authored-by: Valeriu Predoi <[email protected]>
I went over the remaining comments for now on the diagnostic Python file. The code being mostly duplicated and adapted from Doug's repository, it might make it hard to maintain the diagnostic working if some changes are incoming from https://github.com/douglask3/Bayesian_fire_models/tree/AR7_REF, but this is the best we can do for now! Now it's all yours @douglask3 to have a look if I properly transferred your code and if the results look good. You can also answer some remaining opened comments from @valeriupredoi for some clarifications (otherwise I will close them). If there is a need to update the ConFire model files that are shipped as part of ESMValTool, let me know and I will do that. Once they are also on Zenodo and published, we can set that up as the default option. |
Description
Climate drivers for fire for the REF (Climate-REF/climate-ref#215).
This diagnostic includes/will include:
The diagnostic relies on the processing of fire drivers through the ConFire model (dedicated branch from the GitHub repository available at https://github.com/douglask3/Bayesian_fire_models/tree/AR7_REF). The ESMValTool diagnostic includes only the relevant part of the evaluation code (see https://github.com/douglask3/Bayesian_fire_models/blob/AR7_REF/fire_model/ConFire.py for now) as the model run is done offline beforehand. The corresponding result files are made available through a Zenodo archive (will be published with DOI 10.5281/zenodo.14917244) which can be retrieved inside the diagnostic. The other option is to provide a local directory which contains the necessary model files.
The ConFire model relies on a variety of observational datasets:
This PR:
Before you get started
Checklist
It is the responsibility of the author to make sure the pull request is ready to review. The icons indicate whether the item will be subject to the 🛠 Technical or 🧪 Scientific review.
New or updated recipe/diagnostic
To help with the number of pull requests: