Improve rose edit and simplify include files #1055
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jfrost-mo
added
bug
This comment was marked as resolved.
This comment was marked as resolved.
|
|
jfrost-mo
force-pushed
the
improve_rose_edit
branch
3 times, most recently
from
82e818c to
fada613
Compare
jfrost-mo
force-pushed
the
data_loading_improvements
branch
2 times, most recently
from
79ea3e7 to
645ccac
Compare
jfrost-mo
force-pushed
the
improve_rose_edit
branch
from
a0f7c06 to
bab44a6
Compare
daflack
reviewed
Jan 21, 2025
jwarner8
approved these changes
Jan 22, 2025
This reflects the non-sequential nature of the variable, and how it will soon contain multiple values.
This change switches to a generated rose-meta.conf, so it is much easier to maintain. It also removes a number of obsolete keys, updates descriptions, makes analysis_offset and analysis_period per-model, and removes the s/crimes against rose edit/mapping between model and fieldname/. One of the biggest changes is the move to a simple list for model fields instead of the complicated table affair we had before. Fixes #667 Fixes #808 Fixes #817 Fixes #859 Update example rose-suite.conf
This will cause it to be marked as such on GitHub, and not clutter up pull request diffs.
jfrost-mo
force-pushed
the
improve_rose_edit
branch
from
1bff1c9 to
e22ae85
Compare
This allows comparing different variables from the same plot.
Most of the time this is what you want. You only need to set the offset if data isn't output on the first timestep.
Use PT1H as that is what a lot of data is output at. Urban data is often 15 minutely, but is sufficiently niche to require changing an accessible setting.
daflack
approved these changes
Jan 23, 2025
There was a problem hiding this comment.
Looks good and happy to approve, one minor point of clarification (not sure if you are missing something or not).
Extra thought, which I am happy to be put in a different issue if felt needed: in the q-q plot would that mean we would be specifying the same variable twice as we have variable A and variable B for the different models?
|
Indeed, if you wanted to compare the same variable between different models, you would have to put the variable name in both. It would be good to have a chat about it during the quest next week, to clarify what is desired. |
Also tweak a few help descriptions.
jfrost-mo
force-pushed
the
improve_rose_edit
branch
from
226c5f0 to
e65f217
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a bit of a monster PR, however it is mostly replacements or automatically generated code. In this pull request I've done the following:
MODEL_NUMBERinternal variable toMODEL_IDENTIFIERSto reflect its changing role.As the
rose-meta.confis now automatically generated, I wouldn't bother looking too hard at it. Instead look at therose-meta.conf.jinja2, which it is generated from.Contribution checklist
Aim to have all relevant checks ticked off before merging. See the developer's guide for more detail.