Consolidate base directory for diagnostics (obs, mapping and mask files) into one config option #500

xylar · 2018-12-02T06:48:08Z

This merge consolidates the base directory for diagnostics in each config file into one baseDirectory option in the new [diagnostics] section. This means there is only one option that needs to be changed in order to point to the diagnostics directory on each machine instead of needing to set this directory in 4 or 5 different places.

The new [diagnostics] section includes the baseDirectory as well as the mapping and region-masks subdirectories, using the standard directory layout as on the E3SM diagnostics server.

The various *Observations sections (for ocean, sea ice and icebergs) have also been modified to reference this new standard base directory so users will need to change very little to set up a new config file.

Many tasks and quite a bit of documentation needed to be updated to handle these seemingly minor changes.

Example config file have also been updated with new diagnostics section. Various other sections and options were removed as a result of the simplification.

A bug was fixed (for my convenience in debugging the code) in the PlotClimatologyMpasSubtask where decode() was being called on depthSlice even though this is already a string (not an array of bytes) in python 3. This bug shouldn't affect analysis performed with python 2.

xylar · 2018-12-02T06:50:14Z

@milenaveneziani, this came up when I was trying to explain config files to Karthik on his visit last week. It became clear to me that things are unnecessarily complicated now that we have a standard directory structure and should be simplified.

Let me know if you have concerns. It would be great if you could run a test or two but I know you're preparing for AGU just like I am...

milenaveneziani · 2018-12-04T05:40:45Z

@xylar, I just took a quick look. I wonder if it is a good idea to not allow the user to specify the various directories manually, if they want to.
Pinging @vanroekel as well to see if he has an opinion on this.

milenaveneziani · 2018-12-04T05:42:42Z

configs/alcf/config.20171031.tenYearTest.GMPAS-IAF.T62_oEC60to30v3wLI.60layer.theta

-[regions]
-# Directory containing mask files for ocean basins and ice shelves
-regionMaskDirectory = /lus/theta-fs0/projects/ClimateEnergy_2/diagnostics/mpas_analysis/region_masks
-


why is the section below removed from this config file (same goes for other sections in other config files)?

That's the whole point of this PR. People shouldn't need to point to these directories manually now that the directory structure is standardized.

Are you thinking that people would need to make their own region masks and point to those directories? I guess that's conceivable, particularly for non-standard grids.

My thought was that we want the example config files to be relatively simple. I guess you prefer to keep them relatively verbose and to point out to users that they could point the mappingSubdirectory and/or regionMaskSubdirectory as an absolute path?

No, in this case I only meant the section below that has other (unrelated) things in it.
I would only keep config.example more verbose.

I see. The way comments work on GitHub, all I see are the previous 3 lines to your comment so that wasn't clear. It could be that i need to put the MOC section back in but I don't think the rest of this is helpful. It just makes it a lot harder to change default options because all of the example config files have to be changed to match. I think the examples should be relatively lean.

ok, agree that only the MOC section should be put back in for this specific config files, while the others are redundant/arbitrary.

xylar · 2018-12-04T05:46:13Z

@xylar, I just took a quick look. I wonder if it is a good idea to not allow the user to specify the various directories manually, if they want to.

You can still specify the paths to any of the directories as absolute paths, so someone could specify them manually. However, I think we want to discourage folks from having their own directory structure going forward so I don't know that this capability needs to be well advertised.

milenaveneziani · 2018-12-04T05:55:48Z

Well, why not? If the user needs to play with the directory structure for one reason or another, we should let them know they can.
We could just do it in the config.example file (commenting out the specific directory names and mentioning the option).

milenaveneziani · 2018-12-04T05:58:42Z

configs/alcf/config.20180410.A_WCYCL1950_HR.ne120_oRRS18v3_ICG.theta

-[regions]
-# Directory containing mask files for ocean basins and ice shelves
-regionMaskDirectory = /lus/theta-fs0/projects/ClimateEnergy_2/diagnostics/mpas_analysis/region_masks
-


here is another example of section (below this comment) that seems unrelated to this PR and that is being removed. Not sure if it was intentional or not.

I really think we don't want this stuff in the example config files, so, yes, it was intentional.

milenaveneziani · 2018-12-04T05:59:43Z

configs/anvil/config.20170926.FCT2.A_WCYCL1850S.ne30_oECv3.anvil

-
-# directory where sea ice observations are stored
-baseDirectory = /lcrc/group/acme/diagnostics/observations/SeaIce
-


also this piece below about timeSeriesSeaIceAreaVol. And similarly for other config example files.

Actually, it seems this may be the last config file with this issue.

These are all default options being copied into example config files. I realize they might demonstrate to users some of the capabilities that they could customize but we have several hundred such options at this point and these few are just arbitrary in my view. As I said, they make changing defaults very difficult and I think they confuse and overwhelm new users rather than providing helpful direction. That's why I think we want to keep the examples as more the minimum set of options needed to analyze that particular example run and rely on documentation, config.example and config.default to point out additional options.

xylar · 2018-12-04T06:04:27Z

Well, why not? If the user needs to play with the directory structure for one reason or another, we should let them know they can.
We could just do it in the config.example file (commenting out the specific directory names and mentioning the option).

Okay, I think that sounds like a good plan.

xylar · 2018-12-04T06:13:16Z

configs/alcf/config.20171031.tenYearTest.GMPAS-IAF.T62_oEC60to30v3wLI.60layer.theta

-# in its calculation, whereas the postprocessing script does.
-# NOTE: this is a temporary option that will be removed once the online
-# MOC takes into account the bolus velocity when GM is on.
-usePostprocessingScript = True


I will put the MOC section back. That was removed by mistake.

xylar · 2018-12-04T06:29:31Z

@milenaveneziani, thanks very much for all the helpful comments. Sorry if I'm grumpy about them sometimes. And I know I also have strong opinions sometime. But you can always feel free to have strong opinions, too. You are nearly always right when you push me to do things a different way, so I appreciate that.

xylar · 2018-12-04T06:31:00Z

I believe I've addressed the concerns you raised but please let me know if you notice anything more that I missed. I will re-test on my side but I would also appreciate it if you could do a test or 2 yourself, perhaps giving an absolute path to the regions, mapping or observations subdirectories and make sure it does, in fact, work for you.

milenaveneziani · 2018-12-04T06:38:22Z

No problem. Remember, I am a 'stuff keeper' :D

Ok, I will run a test hopefully tomorrow.

xylar · 2018-12-04T14:57:39Z

Ah, right! I should cc Gennaro on these messages. He would understand ;-)

xylar · 2018-12-17T16:17:39Z

@milenaveneziani, have you had a chance to test this one? Are you happy with it now?

This includes a baseDirectory and the mapping and region-masks subdirectories, using the standard directory layout as on the E3SM diagnostics server. The various *Observations sections have also been modified to reference this new standard base directory so users will need to change very little to set up a new config file. Many tasks and quite a bit of documentation needed to be updated to handle these seemlingly minor changes. Example config files will be updated in a separate commit.

Various other sections and options were removed as a result of the simplification.

* Clarify in both that various subdirectories could also be absolute paths * Put these subdirectories in config.example so users can see that they are there to be modified if needed.

mpas_analysis/ocean/climatology_map_eke.py

xylar · 2019-01-10T16:27:46Z

@milenaveneziani, I believe this PR is just waiting on testing from you. Is that correct?

milenaveneziani · 2019-01-10T16:30:28Z

oh cute, there is an actual stickler-ci reviewer! funny.
yes, thanks for the reminder @xylar, I'll test this shortly.

milenaveneziani

My test passed.

xylar · 2019-01-10T22:18:38Z

Excellent! Thank you for testing, @milenaveneziani.

xylar requested a review from milenaveneziani December 2, 2018 06:48

xylar self-assigned this Dec 2, 2018

xylar added bug clean up labels Dec 2, 2018

xylar changed the title ~~Base directory for diagnostics (obs, mapping and mask files) in one config option~~ Consolidate base directory for diagnostics (obs, mapping and mask files) into one config option Dec 2, 2018

milenaveneziani reviewed Dec 4, 2018

View reviewed changes

xylar commented Dec 4, 2018

View reviewed changes

xylar force-pushed the clean_up_diagnostic_dir branch from e981415 to 90deded Compare December 4, 2018 06:26

xylar mentioned this pull request Dec 5, 2018

Remove code related to deprecated "autoclose" option #501

Merged

xylar added 4 commits January 10, 2019 17:16

Update example config file with new diagnostics section

9c796c4

Various other sections and options were removed as a result of the simplification.

Add/fix dignostics sections in test config files

6f827b4

Make config.example and config.default more verbose

06d559e

* Clarify in both that various subdirectories could also be absolute paths * Put these subdirectories in config.example so users can see that they are there to be modified if needed.

xylar force-pushed the clean_up_diagnostic_dir branch from 90deded to 06d559e Compare January 10, 2019 16:16

stickler-ci bot reviewed Jan 10, 2019

View reviewed changes

mpas_analysis/ocean/climatology_map_eke.py Outdated Show resolved Hide resolved

mpas_analysis/ocean/climatology_map_eke.py Outdated Show resolved Hide resolved

mpas_analysis/ocean/climatology_map_eke.py Outdated Show resolved Hide resolved

Fix PEP8 errors

f2b710a

milenaveneziani approved these changes Jan 10, 2019

View reviewed changes

xylar merged commit e178d27 into MPAS-Dev:develop Jan 10, 2019

xylar deleted the clean_up_diagnostic_dir branch January 10, 2019 22:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Consolidate base directory for diagnostics (obs, mapping and mask files) into one config option #500

Consolidate base directory for diagnostics (obs, mapping and mask files) into one config option #500

xylar commented Dec 2, 2018 •

edited

Loading

xylar commented Dec 2, 2018

milenaveneziani commented Dec 4, 2018

milenaveneziani Dec 4, 2018

xylar Dec 4, 2018

milenaveneziani Dec 4, 2018

xylar Dec 4, 2018

milenaveneziani Dec 4, 2018 •

edited

Loading

xylar commented Dec 4, 2018

milenaveneziani commented Dec 4, 2018

milenaveneziani Dec 4, 2018

xylar Dec 4, 2018

milenaveneziani Dec 4, 2018

milenaveneziani Dec 4, 2018

xylar Dec 4, 2018

xylar commented Dec 4, 2018

xylar Dec 4, 2018

xylar commented Dec 4, 2018

xylar commented Dec 4, 2018

milenaveneziani commented Dec 4, 2018

xylar commented Dec 4, 2018

xylar commented Dec 17, 2018

xylar commented Jan 10, 2019

milenaveneziani commented Jan 10, 2019

milenaveneziani left a comment

xylar commented Jan 10, 2019


		# directory where sea ice observations are stored
		baseDirectory = /lcrc/group/acme/diagnostics/observations/SeaIce

Consolidate base directory for diagnostics (obs, mapping and mask files) into one config option #500

Consolidate base directory for diagnostics (obs, mapping and mask files) into one config option #500

Conversation

xylar commented Dec 2, 2018 • edited Loading

xylar commented Dec 2, 2018

milenaveneziani commented Dec 4, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

milenaveneziani Dec 4, 2018 • edited Loading

Choose a reason for hiding this comment

xylar commented Dec 4, 2018

milenaveneziani commented Dec 4, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xylar commented Dec 4, 2018

Choose a reason for hiding this comment

xylar commented Dec 4, 2018

xylar commented Dec 4, 2018

milenaveneziani commented Dec 4, 2018

xylar commented Dec 4, 2018

xylar commented Dec 17, 2018

xylar commented Jan 10, 2019

milenaveneziani commented Jan 10, 2019

milenaveneziani left a comment

Choose a reason for hiding this comment

xylar commented Jan 10, 2019

xylar commented Dec 2, 2018 •

edited

Loading

milenaveneziani Dec 4, 2018 •

edited

Loading