Skip to content
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

Move to one backend #165

Merged
merged 25 commits into from Oct 24, 2018
Merged

Move to one backend #165

merged 25 commits into from Oct 24, 2018

Conversation

znicholls
Copy link
Collaborator

@znicholls znicholls commented Oct 21, 2018

Pull request

Please confirm that this pull request has done the following:

  • Tests added
  • Documentation added (where applicable)
  • Example added (either to an existing notebook or as a new notebook, where applicable)
  • Description in CHANGELOG.rst added

Adding to CHANGELOG.rst

Please add a single line in the changelog notes similar to one of the following:

- (`#XX <http://link-to-pr.com>`_) Added feature which does something
- (`#XX <http://link-to-pr.com>`_) Fixed bug identified in (`#XX <http://link-to-issue.com>`_)

This PR should follow #162

Closes #161 #120

@znicholls
Copy link
Collaborator Author

znicholls commented Oct 21, 2018

@rgieseke I can't work out how to fix the widgets in this, otherwise over to you after #162 is handled

@znicholls znicholls changed the title WIP: Move to one backend Move to one backend Oct 21, 2018
@codecov-io
Copy link

codecov-io commented Oct 21, 2018

Codecov Report

Merging #165 into master will decrease coverage by 1.12%.
The diff coverage is 90.06%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #165      +/-   ##
==========================================
- Coverage   96.99%   95.87%   -1.13%     
==========================================
  Files           5        5              
  Lines        1165     1236      +71     
  Branches      183      201      +18     
==========================================
+ Hits         1130     1185      +55     
- Misses         19       32      +13     
- Partials       16       19       +3
Impacted Files Coverage Δ
pymagicc/definitions/__init__.py 98.47% <100%> (-0.02%) ⬇️
pymagicc/utils.py 100% <100%> (ø) ⬆️
pymagicc/io.py 95.74% <88.4%> (-1.38%) ⬇️
pymagicc/api.py 94.06% <90.54%> (-0.7%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cfce559...b180538. Read the comment docs.

@rgieseke rgieseke closed this Oct 22, 2018
@rgieseke
Copy link
Member

Sorry, GitHub is having issues again ...

pymagicc/__init__.py Outdated Show resolved Hide resolved
pymagicc/api.py Outdated
top_tuning_model = usr_cfg["nml_allcfgs"][top_tuning_model_key]
if top_tuning_model != "PYMAGICC":
raise ValueError(
"PYMAGICC is not top the tuning model in `MAGCFG_USER.CFG`, your run is likely to fail/do odd things"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's "top the tuning model"?

Copy link
Collaborator Author

@znicholls znicholls Oct 23, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm yes this is cryptically named and thinking about it more badly done. I'll change, in the meantime, some context, some of which is not new but I put it all here to try and make sure it's clear (Alex Nauels may also have helpful thoughts as the SLR config is something else again...). Having written this, I'm happy to put this comment in the MAGICC flags docs.

When you run MAGICC, you have a series of .CFG files which set parameter values. The first two are always (and must be) MAGCFG_DEFAULTALL.CFG and MAGCFG_USER.CFG. Exception number 1: in the compiled binary that is shipped with Pymagicc, the file it looks for is MAGCFG_DEFAULTALL_69.CFG, not MAGCFG_DEFAULTALL.CFG.

After these two compulsory files, you can then specify extra .CFG files. Each of these .CFG files is specified in FILE_TUNINGMODEL_X flags.

The first confusing thing: say you have FILE_TUNINGMODEL_X=ABCD, MAGICC will look for MAGTUNE_ABCD.CFG. Note that MAGICC will not look for ABCD or ABCD.CFG. Hence if we have FILE_TUNINGMODEL_8=PYMAGICC, then the 8th tuning file MAGICC will look for will be MAGTUNE_PYMAGICC.CFG. This is why we write the configuration to MAGTUNE_PYMAGICC.CFG and set FILE_TUNINGMODEL_X=PYMAGICC.

The second confusing thing: If FILE_TUNINGMODEL_X is set to USER, MAGICC won't look for any configuration file at all and will simply just move to the next FILE_TUNINGMODEL_X flag. I think the rationale here is that you have to read MAGCFG_USER.CFG (see above) and hence using this flag as a "don't read" flag is safe. Fortunately if you set FILE_TUNINGMODEL_X to an empty string i.e. "" then MAGICC will also skip that .CFG file so there is a safer and more obvious option.

The third confusing thing: the first flag it looks at is FILE_TUNING_MODEL, without any number, whilst the rest are all of the form FILE_TUNINGMODEL_X where X is a positive integer (i.e. not zero).

The fourth confusing thing: the convention above changes in MAGICC7, where FILE_TUNINGMODEL is replaced by FILE_TUNINGMODEL_1

The fifth confusing thing: the maximum value of X in FILE_TUNINGMODEL_X varies depending on your binary and there's no way to query the binary except for just trying higher and higher X until it fails.

The sixth confusing thing: each .CFG file contains a set of parameters, each of which overwrites any previously read parameter values. i.e. the file pointed to by FILE_TUNINGMODEL_4 overwrites flags set by FILE_TUNINGMODEL_3 etc. However, each .CFG file can contain FILE_TUNINGMODEL_X flags itself. Hence you could have the following situation:

  1. MAGCFG_DEFAULTALL.CFG sets FILE_TUNINGMODEL=USER and FILE_TUNINGMODEL_X=USER for all X i.e. no overwrites on top of what will be read from MAGCFG_DEFAULTALL.CFG and MAGCFG_USER.CFG
  2. MAGCFG_USER.CFG sets e.g. FILE_TUNINGMODEL=ZNEXAMPLE and FILE_TUNINGMODEL_2=PYMAGICC, leaving everything else untouched
    • hence at this point we would expect MAGICC to read in MAGCFG_DEFAULTALL.CFG, then overwrite its values with values in MAGCFG_USER.CFG, then overwrite those values with values in MAGTUNE_ZNEXAMPLE.CFG, finally overwriting those values with the values in MAGTUNE_PYMAGICC.CFG
  3. MAGTUNE_ZNEXAMPLE.CFG sets FILE_TUNINGMODEL_2=USER
    • now MAGTUNE_PYMAGICC.CFG won't be read at all

Edit: new example

An even more confusing situation is this one.:

  1. MAGCFG_DEFAULTALL.CFG sets FILE_TUNINGMODEL=USER and FILE_TUNINGMODEL_X=USER for all X i.e. no overwrites on top of what will be read from MAGCFG_DEFAULTALL.CFG and MAGCFG_USER.CFG
  2. MAGCFG_USER.CFG sets e.g. FILE_TUNINGMODEL=ZNEXAMPLE, FILE_TUNINGMODEL_2=RGEX and FILE_TUNINGMODEL_3=PYMAGICC leaving everything else untouched
    • hence at this point we would expect MAGICC to read in MAGCFG_DEFAULTALL.CFG, then overwrite its values with values in MAGCFG_USER.CFG, then overwrite those values with values in MAGTUNE_ZNEXAMPLE.CFG, then overwrite those values with values in MAGTUNE_RGEX.CFG, finally overwriting those values with the values in MAGTUNE_PYMAGICC.CFG
  3. MAGTUNE_ZNEXAMPLE.CFG contains no FILE_TUNINGMODEL_X flags i.e. doesn't change things from our previous expecations
  4. MAGTUNE_RGEX.CFG sets FILE_TUNINGMODEL=USER and FILE_TUNINGMODEL_2=USER. Intuitively, we would expect this to mean that MAGTUNE_ZNEXAMPLE.CFG and MAGTUNE_RGEX.CFG will no longer be read, but actually what will happen is that nothing will change. This is because, when MAGTUNE_RGEX.CFG is read in, it is read in after MAGICC has already looked at the value of the FILE_TUNINGMODEL and FILE_TUNINGMODEL_2 flags and hence altering this flag, at the point in time when MAGTUNE_RGEX.CFG is read in, won't have any futher effect.

The reason this is confusing/annoying is that you have to read, and carefully trace, the hierarchy of every single .CFG file in order to work out what is going to happen. I've put a small function which does this in the scripts directory, it might be worth testing it and including it in Pymagicc for our future sanity.

To avoid any really unexpected, silent surprises, we want the pymagicc cfg file, MAGTUNE_PYMAGICC.CFG (which gets altered to MAGTUNE_PYMAGICC.CFG internally by MAGICC) to overwrite everything else. I'll put a solution which actually does this properly up now.

Whilst we're here, these 'conventions' gets worse when we compare to what happens with the emission scenario files.

In MAGICC6 it's simple, there's only one emissions scenario file and it goes in FILE_EMISSIONSCENARIO=DFGH. MAGICC then looks for files that match DFGH and DFGH.SCEN.

In MAGICC7, there are now multiple FILE_EMISSCEN_X flags (note the shift from FILE_EMISSIONSCENARIO). The values found in the file specified in each FILE_EMISSCEN_X flag overwrite any previously read in values.

Confusing things here:

The first emissions scenario file is specified by FILE_EMISSCEN (without the number), which matches the MAGICC6 convention for FILE_TUNINGMODEL but contradicts the MAGICC7 convention for FILE_TUNINGMODEL.

Say we have FILE_EMISSCEN=DFGH, MAGICC7 looks for files that match DFGH, then DFGH.SCEN7 and then DFGH.SCEN (in that order). Hence the first emissions scenario can be SCEN7, or SCEN, with preference being given to SCEN7 files if there are two scenario files with the same stem (i.e. RCP26.SCEN7 is chosen before RCP26.SCEN if FILE_EMISSCEN=RCP26).

All other FILE_EMISSCEN_X files can only be SCEN7 files. The rationale here (I think) is that SCEN files don't contain easy to read metadata hence overwriting with them is difficult/dangerous.

If you set FILE_EMISSCEN_X=NONE then MAGICC will just move on to the next FILE_EMISSCEN_X flag. However, from above it's clear (and I've tried it) that if you set FILE_TUNINGMODEL_X=NONE, MAGICC will look for MAGTUNE_NONE.CFG, not find it and blow up. Hence there's a direct contradiction there too.

Of course the final part is that each .CFG file can overwrite the FILE_EMISSCEN_X flags of previous .CFG hence working out which scenario will actually be run is also not trivial.

@lewisjared this may be of interest to you

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Viva passed I'd say!

Just a quick thought ... we don't have to support anything that MAGICC7 does in Pymagicc if it's too ... complicated?

Copy link
Collaborator Author

@znicholls znicholls Oct 23, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Viva passed I'd say!

This is material for your PhD proposal.

Just a quick thought ... we don't have to support anything that MAGICC7 does in Pymagicc if it's too ... complicated?

No I think what we do is set things up to run in the simplest way possible and then just fail if they're not how we want them (I'm doing a quick modification now which should show how this can work). Step 2 is to get all this nonsense removed from MAGICC7 before we release it (which is also possible as we can now easily move all the pre-processing into Pymagicc where debugging doesn't require you to learn how to use a Fortran debugger :D).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 for Step 2

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@matthiasmengel @anauels this may interest you.

@rgieseke just added an even more confusing example

pymagicc/api.py Show resolved Hide resolved
pymagicc/api.py Outdated Show resolved Hide resolved
pymagicc/io.py Outdated Show resolved Hide resolved
@znicholls
Copy link
Collaborator Author

Alrighty @rgieseke let's go for round 2 of review please?

@rgieseke
Copy link
Member

Started reading ... quick question: Did you compare the content of PARAMETERS.OUT (in addition to un-changed results for the RCPs?)

pymagicc/api.py Outdated Show resolved Hide resolved
pymagicc/io.py Outdated Show resolved Hide resolved
Copy link
Member

@rgieseke rgieseke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me! Didn't manage to read the tests yet ... feel free to merge!

@znicholls
Copy link
Collaborator Author

Looks good to me! Didn't manage to read the tests yet ... feel free to merge!

There's no rush (about to meet with Matt) so take your time

@znicholls
Copy link
Collaborator Author

Started reading ... quick question: Did you compare the content of PARAMETERS.OUT (in addition to un-changed results for the RCPs?)

No but the default overwriting is sufficiently simple that I'm confident I have got this right (although am happy to do whatever to confirm this)

@znicholls
Copy link
Collaborator Author

@rgieseke 3rd time lucky

.gitignore Outdated Show resolved Hide resolved
Copy link
Member

@rgieseke rgieseke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Except for the minor comment, looks good to me! Let's do this ...

@rgieseke
Copy link
Member

One thing ...
It seems that the new version is slower with the defaults by about 0.5 s:

This branch

%time pymagicc.run(pymagicc.rcp26)
CPU times: user 1.47 s, sys: 123 ms, total: 1.6 s

Pymagicc 1.3.2

%time _ = pymagicc.run(pymagicc.rcp26)
CPU times: user 1.05 s, sys: 87.9 ms, total: 1.14 s

@znicholls
Copy link
Collaborator Author

It seems that the new version is slower with the defaults by about 0.5 s:

Yes I suspect my writer is not a very fast way of doing things. Get it working first, optimise second?

@rgieseke
Copy link
Member

Totally! Merge and let's see how fast we can get it ...

@znicholls znicholls merged commit 4e2d4bb into master Oct 24, 2018
@znicholls znicholls deleted the move-to-one-backend branch October 26, 2018 13:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants