Add groupName as an attribute to merged feature files #67

douglasjacobsen · 2016-10-05T14:48:56Z

This merge adds groupName to the merged feature files, to allow feature
groups to be created by subsequent tools.

Fixes #65

douglasjacobsen · 2016-10-05T14:49:59Z

See #66 (comment) about a conversation related to the motivation / use of groupName.

Note, this doesn't update the driver scripts to automatically set unique / useful groupNames for the resulting feature group files.

pwolfram · 2016-10-05T20:01:44Z

Note, following the group meeting today, @douglasjacobsen mentioned that this should not be as general as in this PR but instead should be included when using the merge script.

douglasjacobsen · 2016-10-05T20:22:46Z

I just pushed an updated version of this branch that does what I think it should in terms of adding groupName to features files.

Note, with the change to json.dump, you'll have to re-merge / dump all of the existing features to prevent people from thinking the scripts have changed them (for example, if you merge any ocean region, and then split it, the entire file is different).

milenaveneziani · 2016-10-05T20:28:14Z

Thanks @douglasjacobsen for doing this.
I think I can test this by running some driver_scripts, right?
Also, I don't think I understood the last 3 lines of Doug's comment: is it something we need to do before merging this PR?

douglasjacobsen · 2016-10-05T20:32:48Z

@milenaveneziani You can test this with two things. 1) run a driver script, and make sure groupName is an attribute. 2) Split some features to where they are supposed to go, and make sure groupName is not an attribute.

You don't need to do anything else for this specific PR, I was just saying that when you do (2) above, the entire file will look like it's changed (because the formatting has changed) so it might be confusing. At some point you guys should re-merge / split all of the features within this repo, to make sure their formatting is consistent with the output format. This is one of the negative side effects of changing the output format.

milenaveneziani · 2016-10-05T20:46:24Z

I see now. I am hoping @xylar will have a better idea of what features will be impacted by this PR.

I ran the setup_MOC_basins driver and the groupName is included in the *_Basin geojson files (which are created with merge_features), but not in the *_MOC geojson's, because those are created with combine_features.py. I can add a similar modification to this script.

milenaveneziani · 2016-10-05T21:34:35Z

Ok, so I added the groupName to the other 2 scripts that are used to create the MOC basins: combine_features and difference_features. Now, if I run setup_MOC_basins, all resulting features have the groupName in them. Same goes for setup_ocean_basins, as it should be.

I will let @xylar take a look at this though before merging.

pwolfram · 2016-10-05T22:42:06Z

@milenaveneziani, I think we will ultimately want "enterGroupName" to be specified via a command line argument, e.g., -g groupName in each of the scripts. I'm happy to make this change or you can, please let me know either way.

milenaveneziani · 2016-10-06T05:08:22Z

@pwolfram: I thought of adding the -g option too. Here is my attempt. Let me know if you have comments.
I also added a specific groupName for MOC and Basins regions in setup_MOC_basins and setup_ocean_basins. Not sure if this should be made an option. I thought against it being an option to reduce degrees of freedom when writing the plotting/analysis script.

milenaveneziani · 2016-10-06T05:38:01Z

Also, I ran split_feature on all *_Basin_separate feature files I had created with setup_ocean_basins and setup_MOC_basins, so that 1) I made sure that groupName was not an attribute in the resulting split features, and 2) I changed the format to most of the features in ocean/region. I ran split_features also on the remaining (11) ocean/region/geojson files that were left with the old format, so now all features on my local branch have the new format.
I am not committing these new features quite yet, though, to double check with you if this is OK.

pwolfram · 2016-10-06T12:45:56Z

@milenaveneziani, I think this makes sense and if something seems like it could be improved I commented on it in the review. Note, we need to do the

Note, with the change to json.dump, you'll have to re-merge / dump all of the existing features to prevent people from thinking the scripts have changed them (for example, if you merge any ocean region, and then split it, the entire file is different).

step @douglasjacobsen recommended (reprinted above for clarity). Note this is up for debate as to whether this should be done here or in a later PR, but I think we want to make this a giant merge for clarity. @xylar, please feel free to comment on this if you disagree.

pwolfram

What was the purpose of the change to ocean/region/Southern_Ocean/region.geojson? We eventually need to update all the files to correspond to the changes with this script but need to discuss whether this should be now or in a later PR. I'm leaning toward now as opposed to later because then all the changes in this PR are consistent and not inter-dependent on another PR but my initial reaction was the opposite. What do you think about this @milenaveneziani?

If you agree, you should push the changes to this repo for review. However, we have already changed the output format in #52 so it wouldn't be a big deal to submit all the changes in a separate PR. In hindsight, I think @xylar and I should have updated all the geojson files when we completed #52.

pwolfram · 2016-10-06T12:32:06Z

merge_features.py

    del paths

+#if ( 'groupName' not in all_features.keys() ):
+#    all_features['groupName'] = "enterGroupName"


Please remove commented text because it is not needed.

pwolfram · 2016-10-06T12:33:10Z

driver_scripts/setup_MOC_basins.py

 options, args = parser.parse_args()

+groupNameMOC = 'MOCRegionsGroup'
+groupNameBasins = 'BasinRegionsGroup'


I'm wondering if there isn't a choice of better name for groupNameBasins. We probably want to make sure the group names are unique throughout the repo too.

What about OceanBasinsGroup?
I wanted to leave Regions in groupNameMOC because at some point we will also have MOCTransectsGroup.
Also, this is the only place where we define specific groups for now.

Another 2 regions that should fall in the OceanBasinsGroup are the GlobalOcean_* features: any suggestion in whether this should be done by hand or within the setup_ocean_basins driver?

I think the group name needs to be different than in driver_scripts/setup_ocean_basins.py and other files. Maybe groupNameBasins = 'BasinRegionsGroup' to groupNameBasins = 'MOCBasinRegionsGroup'?

I think the files should be entirely produced by the scripts to document how they are created.

To summarize:

groupNames should be unique from each script

I would include elements like MOC, Global, etc. as well as Regions and Transects for greater specificity in groupNames

pwolfram · 2016-10-06T12:34:05Z

combine_features.py

                    metavar="NAME", required=True)
+parser.add_argument("-g", "--groupName", dest="groupName",
+                    help="Feature group name.",
+                    metavar="GROUPNAME", default="enterGroupName")


Instead of "enterGroupName" I think we want something like "unspecifiedGroupName" and this should be a default throughout the changes to this repo.

pwolfram · 2016-10-06T12:34:34Z

difference_features.py

                    metavar="FILE2", required=True)
+parser.add_argument("-g", "--groupName", dest="groupName",
+                    help="Feature group name.",
+                    metavar="GROUPNAME", default="enterGroupName")


Please see previous comment about changing default.

pwolfram · 2016-10-06T12:35:07Z

driver_scripts/setup_ocean_basins.py


 options, args = parser.parse_args()

+groupName = 'BasinRegionsGroup'


This groupName is not unique with respect to setup_MOC_basins.py.

Why do you think it is not unique?

The name sounded too generic and not specific enough. Maybe something like GlobalOceanBasinRegionsGroup would be better?

pwolfram · 2016-10-06T12:35:19Z

merge_features.py

 parser.add_argument("-f", "--feature_file", dest="feature_file", help="Single feature file to append to features.geojson", metavar="FILE")
 parser.add_argument("-d", "--features_directory", dest="features_dir", help="Directory containing multiple feature files, each will be appended to features.geojson", metavar="PATH")
+parser.add_argument("-g", "--group", dest="groupName", help="Feature group name.", metavar="GROUPNAME", default="enterGroupName")
 parser.add_argument("-t", "--tags", dest="tags", help="Semicolon separated list of tags to match features against.", metavar='"TAG1;TAG2;TAG3"')


Please see previous comment about changing default.

xylar · 2016-10-06T13:37:50Z

What was the purpose of the change to ocean/region/Southern_Ocean/region.geojson?

I am wondering the same. @douglasjacobsen included this in his commit, but I'm wondering if that was by mistake and should be undone.

@milenaveneziani has also undone @douglasjacobsen's only other change, so maybe the first commit on this branch should be rebased out.

We eventually need to update all the files to correspond to the changes with this script but need to discuss whether this should be now or in a later PR. I'm leaning toward now as opposed to later because then all the changes in this PR are consistent and not inter-dependent on another PR but my initial reaction was the opposite. What do you think about this @milenaveneziani?

If you agree, you should push the changes to this repo for review. However, we have already changed the output format in #52 so it wouldn't be a big deal to submit all the changes in a separate PR. In hindsight, I think @xylar and I should have updated all the geojson files when we completed #52.

I strongly feel like this needs to be a separate PR. I really dislike when PRs creep way beyond their initial intended purpose just because there is a need for other clean up. While I suppose this cleanup should have happened as part of #52, I don't think we split features very often at this point because most of the features we want to define are now in the repo, so I didn't see much urgency in performing this update when I posted #52.

pwolfram · 2016-10-06T13:48:07Z

Following @xylar's comments I propose we don't change any geojson files in this repo and move that we save geojson changes to a fresh PR.

milenaveneziani · 2016-10-06T15:23:53Z

@pwolfram and @xylar: about the Souther_Ocean geojson file change, I was also puzzled by why that happened. But I did mention in one of my comments that I have run split_features (as Doug suggested) on the *_Basin_separated files and all the remaining features, so I now have the newer format on my local branch.
If you prefer these changes to go in a different PR, could you please suggest how to undo the current change in the SO geojson file?

pwolfram · 2016-10-06T16:59:29Z

@milenaveneziani, I'll take care of removing the SO geojson file as we discussed.

pwolfram · 2016-10-06T17:49:28Z

@milenaveneziani, we observe the problem with Atlantic_Basin.geojson produced by setup_ocean_basins.py and setup_MOC_basins.py yielding different output but having the same groupName.

The coordinates are also very different. This is something to keep in mind when using these results in the model because I would say that having the same groupName point to different features is dangerous long term.

The more I think about it we need to have a different groupName for each script and I would argue that this has to happen in this PR, not another one, because we are grappling with the groupName issue here.

pwolfram · 2016-10-06T17:50:17Z

In summary, one of OceanBasinRegionsGroup needs renamed.

milenaveneziani · 2016-10-06T18:07:28Z

@pwolfram: now this is confusing to me too.. I really thought the two Atlantic_Basin features generated by the 2 drivers were the same. I would say let us have a unique groupName for this PR, but we need to address this asap. I was just about to open an issue about this.

milenaveneziani · 2016-10-06T18:13:45Z

@pwolfram: would you mind making the change and pushing, since you are already doing some clean up on your side? thanks.

Added groupName and option to specify groupName in merge_, combine_ and difference_features

douglasjacobsen added the enhancement label Oct 5, 2016

douglasjacobsen assigned xylar, pwolfram and milenaveneziani Oct 5, 2016

douglasjacobsen force-pushed the add_group_name branch from f5a74b1 to b81c487 Compare October 5, 2016 20:21

pwolfram suggested changes Oct 6, 2016

View reviewed changes

milenaveneziani added a commit to douglasjacobsen/geometric_features that referenced this pull request Oct 6, 2016

Revisions for PR MPAS-Dev#67

f6f5f76

pwolfram force-pushed the add_group_name branch from f6f5f76 to 70c3256 Compare October 6, 2016 17:24

pwolfram approved these changes Oct 6, 2016

View reviewed changes

pwolfram mentioned this pull request Oct 6, 2016

setup_MOC_basins re-defines three Basin regions that are already defined in setup_ocean_basins #70

Closed

Adds groupName capability to scripts

57a6a31

Added groupName and option to specify groupName in merge_, combine_ and difference_features

pwolfram force-pushed the add_group_name branch from 70c3256 to 57a6a31 Compare October 6, 2016 20:15

pwolfram merged commit 57a6a31 into MPAS-Dev:master Oct 6, 2016

This was referenced Oct 15, 2016

default groupName needs to be consistent with what MPAS mask creator expects #72

Closed

Consolidate output from driver scripts into single geojson files #73

Closed

Change how groupName is set; update setup_ocean_basins #74

Merged


		options, args = parser.parse_args()

		groupName = 'BasinRegionsGroup'

Add groupName as an attribute to merged feature files #67

Add groupName as an attribute to merged feature files #67

Uh oh!

Conversation

douglasjacobsen commented Oct 5, 2016

Uh oh!

douglasjacobsen commented Oct 5, 2016

Uh oh!

pwolfram commented Oct 5, 2016

Uh oh!

douglasjacobsen commented Oct 5, 2016

Uh oh!

milenaveneziani commented Oct 5, 2016

Uh oh!

douglasjacobsen commented Oct 5, 2016

Uh oh!

milenaveneziani commented Oct 5, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

milenaveneziani commented Oct 5, 2016

Uh oh!

pwolfram commented Oct 5, 2016

Uh oh!

milenaveneziani commented Oct 6, 2016

Uh oh!

milenaveneziani commented Oct 6, 2016 • edited by xylar Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pwolfram commented Oct 6, 2016

Uh oh!

pwolfram left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

milenaveneziani Oct 6, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xylar commented Oct 6, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pwolfram commented Oct 6, 2016

Uh oh!

milenaveneziani commented Oct 6, 2016

Uh oh!

pwolfram commented Oct 6, 2016

Uh oh!

pwolfram commented Oct 6, 2016

Uh oh!

pwolfram commented Oct 6, 2016

Uh oh!

milenaveneziani commented Oct 6, 2016

Uh oh!

milenaveneziani commented Oct 6, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

milenaveneziani commented Oct 5, 2016 •

edited

Loading

milenaveneziani commented Oct 6, 2016 •

edited by xylar

Loading

milenaveneziani Oct 6, 2016 •

edited

Loading

xylar commented Oct 6, 2016 •

edited

Loading