-
Notifications
You must be signed in to change notification settings - Fork 13
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
code to make the cams2_83 mos evaluation work #1166
Conversation
…starting point is supposed to be pre-made colocated data, hence the only_json-no-obs-read logic
…incide with coldata_dir perhaps
…om colocated data to work
… too for colocated data in place in the coldata_path to be read and processed, tested with a run on PPI
… too for colocated data in place in the coldata_path to be read and processed, tested with a run on PPI
…S2_83_Processer, it used col.get_available_coldata_files(var_list), which reads data only from standard subfolders, which are not the relevant ones for the median scores
…em to the ModelName class
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main-dev #1166 +/- ##
============================================
+ Coverage 78.75% 78.79% +0.04%
============================================
Files 127 128 +1
Lines 20105 20164 +59
============================================
+ Hits 15833 15889 +56
- Misses 4272 4275 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
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 good, just some minor comments.
pyaerocom/scripts/cams2_83/cli.py
Outdated
logger.warning( | ||
f"The given pool {pool} is larger than the maximum CPU count {mp.cpu_count()}. Using that instead" | ||
) | ||
pool = mp.cpu_count() |
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.
You can give a warning, but let the user shoot in his own foot.
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.
how about setting it to 1 then?
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.
nvm, I like your approach best
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 can create 5000 processes without problems on my 6-core machine. Why not just let the user do what he asked for, even if it does not make sense to you?
pyaerocom/scripts/cams2_83/cli.py
Outdated
"--pool", | ||
"-p", | ||
min=1, | ||
max=cpu_count(), |
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.
Don't set cpu_count() as maximum if you don't know if this is right.
Some tasks are IO bound, so a maximum of 4 is often too much in this case. In theory, we could have tasks which connect to external resources, so that we might want to use much more than the cpus available. (A process based webserver setup used to be 2-4 times the number of cores.)
We don't need to add a maximum here.
pyaerocom/scripts/cams2_83/cli.py
Outdated
"--pool", | ||
"-p", | ||
min=1, | ||
max=cpu_count(), |
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.
remove, see above
"maps.php", | ||
"intercomp.php", | ||
"overall.php", | ||
] |
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.
Do we still write .php pages? I thought @AugustinMortier had removed those?
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 have no idea but this is main-dev code, so I'd rather address that somewhere else than in this PR
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
…ead of subcommands, so that we do not break the commands deployed in production now for the standard experiments
tested with a MOS/ENS evaluation run |
Change Summary
add code to make it possible to run the MOS/ENS evaluation starting from pre-made colocated data
this addresses https://github.com/metno/AeroToolsIssues/issues/1
Checklist