valeriupredoi / bgcval2

Package for BGCVal v2.0
3 stars 0 forks source link

New boolean in analysis_compare to run report maker only #102

Closed ledm closed 11 months ago

ledm commented 11 months ago

Sometimes, we need to just make a report instead of running the analysis_timeseries code as well. It's possible to change this in the input_yml file, but this runtime bool overwrites that function.

Basically, this means that we can produce a report without changing the yml, which is handy while running several things at once.

At the same time, I've added several regions that Colin requested and included the new historical recipe.

Closes #103 Closes #104 Closes #105

ledm commented 11 months ago

Hey @valeriupredoi, no rush on this, but I wound up doing two things at once. Figured a merge could do both.

ledm commented 11 months ago

Wait a second, just found out that the parser isn't done properly. I'm going to implement it as as BooleanOptionalAction instead.

parser.add_argument('--feature', default=True, action=argparse.BooleanOptionalAction)
valeriupredoi commented 11 months ago

@ledm take your time! If you wish, you can always mark the PR as Draft while you still devving it :beer:

ledm commented 11 months ago

Okay, pretty sure it's behaving the way I want it to now. Ready for review.

valeriupredoi commented 11 months ago

this RfR bud? :beer:

ledm commented 11 months ago

Yeah, go for it!

I wouldn't merge until @DrYool gives it a go though, as he raised the issues it solves.

valeriupredoi commented 11 months ago

wait - am a bit confused is --skip-timeseries accepting a value in the command eg --skip-timesries True or --skip-timeseries False? or is it a bool ie when --skip-timesries in command line that means skip analysis_timeseries, and when not present at all skip_timeseries is None

valeriupredoi commented 11 months ago

also, happy to see Starting analysis_timeseries is not happening with --skip_timesries but there is still some model data loading in the test, with output:

analysis-Timeseries.py: Beginning to call timeseriesAnalysis for  Temperature_integration_test
timeseriesAnalysis:      init.
timeseriesAnalysis:     loadModel.
Does not exist /home/valeriu/bgcval2/local_test/BGC_data/valeriu/bgcval2/shelves/timeseries/u-cp647debug/u-cp647debug_Temperature_integration_test.shelve
timeseriesAnalysis:     loadModel       Opening shelve: /home/valeriu/bgcval2/local_test/BGC_data/valeriu/bgcval2/shelves/timeseries/u-cp647debug/u-cp647debug_Temperature_integration_test.shelve
KeysView(<shelve.DbfilenameShelf object at 0x7f8342954b90>)
timeseriesAnalysis:     loadModel       Checking:  ['Global', 'Surface', 'mean']         has  1 keys
timeseriesAnalysis:     loadModel:      post checks...
timeseriesAnalysis:     loadModel       shelveFn: /home/valeriu/bgcval2/local_test/BGC_data/valeriu/bgcval2/shelves/timeseries/u-cp647debug/u-cp647debug_Temperature_integration_test.shelve
timeseriesAnalysis:     loadModel       readFiles: contains  1 files.   Up to  /home/valeriu/bgcval2/local_test/BGC_data/u-cp647debug/nemo_u-cp647debugo_1y_1990_grid-T.nc
timeseriesAnalysis:     loadModel.      no New Files requested. Loaded:  1 Model data

Is that supposed to be there?

ledm commented 11 months ago

wait - am a bit confused is --skip-timeseries accepting a value in the command eg --skip-timesries True or --skip-timeseries False? or is it a bool ie when --skip-timesries in command line that means skip analysis_timeseries, and when not present at all skip_timeseries is None

skip_timeseries sets a bool which means that you don't analyse any new data, but you do load the old ones. This means that is is exactly the behaviour we expect.

timeseriesAnalysis: loadModel. no New Files requested. Loaded: 1 Model data

The confusing part is that the overwrites a bool in the input_yml, which can be True or False. So, there's 3 options:

valeriupredoi commented 11 months ago

OK so in terms of the actual command line flag:

No other values given to the cmd line flags, no?

ledm commented 11 months ago

OK so in terms of the actual command line flag:

  • None (no command line flag): Do whatever is in the input_yml -> ""
  • True: Skip new data in timeseriesAnalysis. -> "--skip_timeseries"
  • False: Force it to look for an analyse it. -> "--no-skip_timesries"

No other values given to the cmd line flags, no?

Yes, this is accurate.

valeriupredoi commented 11 months ago

skip_timeseries sets a bool which means that you don't analyse any new data, but you do load the old ones. This means that is is exactly the behaviour we expect.

Gotcha! Awesome, makes sense then :beers:

valeriupredoi commented 11 months ago

That's cool, we have tests to test these new options, but am still a bit weirded out why my local tests fail unless that assert 0 gets removed

ledm commented 11 months ago

Okay, we got the ok from @DrYool, go ahead and merge, @valeriupredoi!

valeriupredoi commented 11 months ago

Brilliant, many thanks @DrYool for testing (and closing all the other issues, it's always nice when users close their issues when things work well for them!) and @ledm for the nice enhancement :beer: x2