New boolean in analysis_compare to run report maker only #102

ledm · 2023-08-07T13:53:17Z

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 · 2023-08-07T13:53:52Z

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

ledm · 2023-08-07T14:34:36Z

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 · 2023-08-07T14:50:32Z

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

ledm · 2023-08-07T15:02:50Z

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

README.md

ledm

Fix commits

key_files/alkalinity.yml

key_files/atmospco2.yml

key_files/chlorophyll.yml

key_files/dic.yml

key_files/dust.yml

key_files/nitrate.yml

key_files/oxygen.yml

key_files/salinity.yml

key_files/silicate.yml

key_files/temperature.yml

…to dev_report_only Conflicts: key_files/dust.yml

valeriupredoi · 2023-08-10T14:08:01Z

this RfR bud? 🍺

ledm · 2023-08-10T14:09:09Z

Yeah, go for it!

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

valeriupredoi

looks good, let me add an API test and - like you say - wait for @DrYool to give you green light 🟢

bgcval2/analysis_compare.py

valeriupredoi

@ledm one hitch - the assert 0 is causing the local tests (with a fresh env) to fail - could you pls remove it, or place a valid exception that needs be raised? 🍺

bgcval2/bgcval2_make_report.py

valeriupredoi · 2023-08-10T15:05:37Z

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 · 2023-08-10T15:25:08Z

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?

Co-authored-by: Valeriu Predoi <valeriu.predoi@gmail.com>

ledm · 2023-08-10T15:33:07Z

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:

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

valeriupredoi · 2023-08-10T15:47:44Z

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?

ledm · 2023-08-10T15:48:37Z

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 · 2023-08-10T15:48:48Z

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 🍻

valeriupredoi · 2023-08-10T15:49:54Z

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

valeriupredoi

tests pass locally (on me home machine) with both Python=3.10 and 3.11 no problem (and of course on GA machine), awesome work @ledm (very possible me work machine, having an old OS, is messing up due to old kernel) 🍻

ledm · 2023-08-14T10:54:45Z

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

valeriupredoi · 2023-08-14T11:29:17Z

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 🍺 x2

ledm added 3 commits August 7, 2023 14:40

added new rejoins and new feature

11424a3

Merge remote-tracking branch 'origin/main' into hovmoeller

a317364

Added new tf run.

5be3496

ledm requested a review from valeriupredoi August 7, 2023 13:53

added new regions.

cb9dec0

Re-did boolean command line arg.

1e6f23c

ledm commented Aug 9, 2023

View reviewed changes

README.md Outdated Show resolved Hide resolved

ledm commented Aug 9, 2023

View reviewed changes

README.md Outdated Show resolved Hide resolved

ledm commented Aug 9, 2023

View reviewed changes

README.md Outdated Show resolved Hide resolved

ledm mentioned this pull request Aug 9, 2023

Missing geographical regions #103

Closed

ledm commented Aug 9, 2023

View reviewed changes

ledm added 6 commits August 9, 2023 12:10

Update key_files/alkalinity.yml

1da18aa

Apply suggestions from code review

ca8ba28

local copy

beabca4

Merge branch 'dev_report_only' of github.com:valeriupredoi/bgcval2 in…

8d4a90b

…to dev_report_only Conflicts: key_files/dust.yml

Solved #104 and #105

3feb196

Some bug fixes.

2a33236

ledm requested a review from DrYool August 9, 2023 14:41

ledm mentioned this pull request Aug 9, 2023

Pep8 non-compliance and minor bugs. #106

Open

2 tasks

valeriupredoi approved these changes Aug 10, 2023

View reviewed changes

bgcval2/analysis_compare.py Outdated Show resolved Hide resolved

add case with new cmd line arg

ae6fe0e

valeriupredoi requested changes Aug 10, 2023

View reviewed changes

bgcval2/bgcval2_make_report.py Show resolved Hide resolved

valeriupredoi and others added 2 commits August 10, 2023 16:25

run test with no-skip too

d576930

Update bgcval2/analysis_compare.py

c79c045

Co-authored-by: Valeriu Predoi <valeriu.predoi@gmail.com>

valeriupredoi approved these changes Aug 11, 2023

View reviewed changes

Merge branch 'main' into dev_report_only

db4b9f1

valeriupredoi merged commit a775ec0 into main Aug 14, 2023

valeriupredoi deleted the dev_report_only branch August 14, 2023 11:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New boolean in analysis_compare to run report maker only #102

New boolean in analysis_compare to run report maker only #102

ledm commented Aug 7, 2023 •

edited

Loading

ledm commented Aug 7, 2023

ledm commented Aug 7, 2023

valeriupredoi commented Aug 7, 2023

ledm commented Aug 7, 2023

ledm left a comment

valeriupredoi commented Aug 10, 2023

ledm commented Aug 10, 2023

valeriupredoi left a comment

valeriupredoi left a comment

valeriupredoi commented Aug 10, 2023 •

edited

Loading

valeriupredoi commented Aug 10, 2023

ledm commented Aug 10, 2023

valeriupredoi commented Aug 10, 2023 •

edited

Loading

ledm commented Aug 10, 2023

valeriupredoi commented Aug 10, 2023

valeriupredoi commented Aug 10, 2023

valeriupredoi left a comment

ledm commented Aug 14, 2023

valeriupredoi commented Aug 14, 2023

New boolean in analysis_compare to run report maker only #102

New boolean in analysis_compare to run report maker only #102

Conversation

ledm commented Aug 7, 2023 • edited Loading

ledm commented Aug 7, 2023

ledm commented Aug 7, 2023

valeriupredoi commented Aug 7, 2023

ledm commented Aug 7, 2023

ledm left a comment

Choose a reason for hiding this comment

valeriupredoi commented Aug 10, 2023

ledm commented Aug 10, 2023

valeriupredoi left a comment

Choose a reason for hiding this comment

valeriupredoi left a comment

Choose a reason for hiding this comment

valeriupredoi commented Aug 10, 2023 • edited Loading

valeriupredoi commented Aug 10, 2023

ledm commented Aug 10, 2023

valeriupredoi commented Aug 10, 2023 • edited Loading

ledm commented Aug 10, 2023

valeriupredoi commented Aug 10, 2023

valeriupredoi commented Aug 10, 2023

valeriupredoi left a comment

Choose a reason for hiding this comment

ledm commented Aug 14, 2023

valeriupredoi commented Aug 14, 2023

ledm commented Aug 7, 2023 •

edited

Loading

valeriupredoi commented Aug 10, 2023 •

edited

Loading

valeriupredoi commented Aug 10, 2023 •

edited

Loading