Added option to write out the cellMap variable to a text file #149

akturner · 2016-11-18T05:06:27Z

In creating new partition files for the sea ice model I have found the need to write out the cellMap variable to a text file during cell culling. This PR adds this capability to the cell culler. getopt is now used to handle program options. Input and output grid files are now specified with the -i and -o options. The original -i (invert) option is changed to -v. Writing out the cellMap variable is specified with the -n option. The -h option now prints the usage. This PR attempts to implement comments from @xylar on a previous attempt to implement this functionality.

Changed cellCuller options to use getopt Added -n option to output cellMap variable to a text file Added -i/-o options to input and output filename options Changed the -i option to be -v since -i seemed better to use with input filenames Added -h option to output usage

xylar · 2016-11-18T11:23:23Z

grid_gen/mesh_conversion_tools/mpas_cell_culler.cpp

+	}

+	// default for input
+	if (in_name == "") {


@akturner, to be consistent with print_usage, and to be backwards compatible with existing ocean test cases that assume default values of the arguments this and the next two if statements should be removed (in_name and out_name should be given their default values, not be read in from stdin). In ocean test cases, we assume that the input and/or output arguments will be the defaults and we definitely do not want the script to prompt for a file name when we're running a script that includes calls to the cell culler.

In general, I think these tools need to avoid using stdin (cin) because we plan to use them as part of automated scripts.

@xylar The intent here was to make this as similar to the original code as possible. Using getopt it doesnt appear the exact behavior of the original code is exactly reproducible. Personally, I dont see the utility in using stdin and cin and am happy to remove it.

I see. Weird. So the original code would ask for an input file name if you didn't give one (so that argument wasn't actually optional) but didn't to that for the output argument. I don't think that makes any sense.

I would strongly recommend getting rid of anything to do with cin.

I have removed the ability to read in the filenames from the stdin.

xylar · 2016-11-18T11:27:19Z

grid_gen/mesh_conversion_tools/mpas_cell_culler.cpp

-	cout << "\t\t-m/-i:" << endl;
+	cout << "\t\t\tIf not specified, it defaults to culled_mesh.nc." << endl;
+	cout << "\t\t-m/-v/-p:" << endl;
 	cout << "\t\t\tThis argument controls how a set of masks is used when culling a mesh." << endl;


Should now be "These arguments"

I've corrected this.

xylar · 2016-11-18T11:29:40Z

grid_gen/mesh_conversion_tools/mpas_cell_culler.cpp

-	cout << "\t\t\tThe -i argument applies the inverse mask to cull based on (i.e. where the mask is 0, the mesh will be culled)." << endl;
+	cout << "\t\t\tThe -v argument applies the inverse mask to cull based on (i.e. where the mask is 0, the mesh will be culled)." << endl;
 	cout << "\t\t\tThe -p argument forces any marked cells to not be culled." << endl;
 	cout << "\t\t\tIf this argument is specified, the masks_name argument is required" << endl;


Should be "For each of these arguments that is specified," (since multiple of these can be specified at once).

I've corrected this.

xylar · 2016-11-18T11:30:53Z

grid_gen/mesh_conversion_tools/mpas_cell_culler.cpp

+	cout << "\t\t\tThe -v argument applies the inverse mask to cull based on (i.e. where the mask is 0, the mesh will be culled)." << endl;
 	cout << "\t\t\tThe -p argument forces any marked cells to not be culled." << endl;
 	cout << "\t\t\tIf this argument is specified, the masks_name argument is required" << endl;
+	cout << "\t\t\tIf one of the mask options are specified both the input and output filenames must be explicitly specified." << endl;


I do not think this is a requirement, since they will default to "grid.nc" and "culled_mesh.nc" if they are not specified.

I've corrected this.

xylar · 2016-11-18T11:33:05Z

grid_gen/mesh_conversion_tools/mpas_cell_culler.cpp


 int main ( int argc, char *argv[] ) {
 	int error;
-	string out_name = "culled_mesh.nc";


Why change this behavior when you use a pair of if statements to do the same below?

Removing stdin greatly simplified the options and set the defaults to grid.nc and culled_mesh.nc if -i or -o are not specified

xylar · 2016-11-18T11:33:48Z

grid_gen/mesh_conversion_tools/mpas_cell_culler.cpp

 	}

+	if (out_name == "") {
+		out_name = "culled.nc";


This is not the default name given in print_usage. It is also not the default name expected by a large number of ocean test cases that use the cell culler. I would prefer that it remain 'culled_mesh.nc' for backwards compatibility.

I've corrected this.

xylar · 2016-11-18T11:36:18Z

@akturner, I haven't had a chance to test the code so far, but I'll get to that shortly. None of my comments so far relate to the normal usage of the code, just to print_usage and the behavior when the input and/or output file names are not specified.

xylar · 2016-11-18T11:37:52Z

Since almost every ocean test case uses the cell culler, we will need a PR into ocean/develop to make all test cases compatible with the new -i and -o flags when they get merged from develop to ocean/develop. I will take care of that when the time comes.

xylar · 2016-11-18T11:52:16Z

grid_gen/mesh_conversion_tools/mpas_cell_culler.cpp

+
+	// need explicit input and output names if mask flags specified
+	if (cullMasks and (in_name == "" or out_name == "")) {
+		printf ("ERROR: Need explicit input and output names if mask flags specified.\n", c);


The second argument to printf (c) isn't needed (and is giving me warnings when I compile the code).

This has been removed.

xylar · 2016-11-18T12:40:40Z

I verified that this version of the cell culler works fine with the ocean/global_ocean/QU_240km/performance_test test case on ocean/develop when I add the -i and -o flags, and change the existing -i to a -v flag for calls to the cell culler in the init_step1.py script.

Removed the ability to read in filenames from stdin. The code now defaults to grid.nc for input and culled_mesh.nc for output. Corrected the default output to culled_mesh.nc Updated the usage statement.

akturner · 2016-11-18T13:59:49Z

@xylar Thanks for these suggestions. I've corrected the code as requested. Thanks for reviewing this.

akturner · 2016-11-18T14:00:39Z

Would there be utility in updating the mask_creator and mesh_converter to use getopt and to be consistent with the usage of -i and -o here?

xylar · 2016-11-18T14:23:12Z

Would there be utility in updating the mask_creator and mesh_converter to use getopt and to be consistent with the usage of -i and -o here?

@akturner, I think that would be a very good idea. It would make the 3 tools less confusing, especially for new users. Also, if we want to make this change, it would be good to do so for all the tools at once. That way I can update all the ocean test cases to the new flags in one go.

xylar · 2016-11-18T14:27:44Z

The code looks great to me now.

By the way, I verified that the code writes out a list of cell indices to a text file. Since I don't have any tools that would make use of this mapping, I didn't have a way of verifying that they are the correct indices, but given how dead simple the code is for writing them out, I'm inclined to believe they're what you want.

xylar · 2016-11-18T14:28:53Z

I'll wait for updates on the mask creator and mesh converter, and then I'll be happy to re-check and merge as soon as I've got the go-ahead from @matthewhoffman , @mgduda and @mark-petersen.

mark-petersen · 2016-11-18T14:41:06Z

@xylar thank you for your review and testing of this PR. It is particularly helpful now that I have responsibilities coordinating both MPAS and ACME pull requests.

xylar · 2016-11-18T14:48:30Z

@mark-petersen, happy to do my part.

akturner · 2016-11-18T15:43:19Z

I will work on modifying the other two tools. I will also need to update the README.

Are the test input files for the mesh converter and the mask creator I can use for testing?

xylar · 2016-11-18T16:20:56Z

I will also need to update the README.

Good call.

Are the test input files for the mesh converter and the mask creator I can use for testing?

I'll generate some for you using one of the ocean test cases.

xylar · 2016-11-18T16:45:21Z

@akturner, you should be able to find what you need in the folder:

/lustre/scratch3/turquoise/xylar/mpas_runs/baseline/ocean/global_ocean/QU_240km/performance_test/init_step1

Good tests would be to see if you can reproduce the following sequence of commands (this is the syntax of the old tools) with the new tools, and if you get bit-for-bit identical results (on wolf) to the files I got. You could tar up the contents of the directory and untar it somewhere in your scratch directory, move all the output files (land_mask.nc critical_passages_mask.nc culled_mesh.nc seed_mask.nc culled_mesh_final.nc critical_passages_mask_final.nc test.nc) to a temporary directory to make it easier to do bit-for-bit checks on them, e.g. with diff, and run the tests below.

./MpasMaskCreator.x  base_mesh.nc land_mask.nc -f land_coverage.geojson
./MpasMaskCreator.x  base_mesh.nc critical_passages_mask.nc -f critical_passages.geojson
./MpasCellCuller.x  base_mesh.nc culled_mesh.nc -m land_mask.nc -p critical_passages_mask.nc
./MpasMaskCreator.x  culled_mesh.nc seed_mask.nc -s seed_points.geojson
./MpasCellCuller.x  culled_mesh.nc culled_mesh_final.nc -i seed_mask.nc
./MpasMaskCreator.x  culled_mesh_final.nc critical_passages_mask_final.nc -f critical_passages.geojson
./MpasMeshConverter.x culled_mesh_final.nc test.nc

All these steps except the last one are part of our standard setup for realistic ocean test cases.

Note, I built the tools with intel compilers, so that would make a difference. I could redo with gnu if necessary.

You should have permission to read these files but let me know if that's not the case.

Mesh converter and mask converter now use getopt for command line options. Input and output filenames are specifid with -i and -o flags now. README has been updated to reflect changes Updated print usage statements. Removed trailing whitespace.

akturner · 2016-11-22T16:56:22Z

@xylar I performed those tests and the modified code gave zero differences with ncdiff. I modified the README to reflect the new functionality and removed trailing whitespace.

xylar · 2016-11-22T17:12:33Z

@mgduda and @matthewhoffman, do you use these tools as part of the workflow of your respective components? If not, could you let me know so I don't wait for your response. If so, could you run a test for each tool you use to make sure @akturner's changes don't break anything for you?

matthewhoffman · 2016-11-22T17:48:34Z

We use these tools, and these changes are fine with me. I don't like having to update our test cases for the syntax change, but it's not hard and in the long run I can see how these changes will be worthwhile.

Note (as is probably obvious) that changes like this reveal one of the remaining vulnerabilities in the test case infrastructure - this change requires ensuring two different repos (MPAS and MPAS-Tools) are synced across this change, otherwise tests break. This might make regression tests or git bisect painful for a little while.

xylar · 2016-11-22T19:55:30Z

I don't like having to update our test cases for the syntax change, but it's not hard and in the long run I can see how these changes will be worthwhile.

I agree on both points. It's going to be a pain on the ocean side, too, for a bit. But we'll make due.

Note (as is probably obvious) that changes like this reveal one of the remaining vulnerabilities in the test case infrastructure - this change requires ensuring two different repos (MPAS and MPAS-Tools) are synced across this change, otherwise tests break. This might make regression tests or git bisect painful for a little while.

That's right. I had been thinking somehow that there would be a merge (like develop to ocean/develop) at which point we could synchronize these changes. That's not the case. Which means that I don't actually think we can merge these changes until after an ACME v1 release branch is created from ocean/develop. Otherwise, we're going to have to make another merge into ocean/develop to be compatible with these changes.

@mark-petersen (and anyone else), what are your thoughts?

xylar · 2016-11-23T12:06:18Z

cc @mark-petersen

I made a PR (MPAS-Dev/MPAS#1148) with the idea that it and this PR should be fulfilled essentially simultaneously. @matthewhoffman, we will want a similar branch and PR for landice/develop.

Again, the question remains whether we need to create release branches of this repo, ocean/develop and landice/develop before making these changes. ACME should be unaffected but the policy has been essentially not to make unnecessary changes in the develop branches until we have a release branch...

matthewhoffman · 2016-11-23T15:19:12Z

@xylar , another short term solution would be to avoid updating any local checkouts of MPAS-Tools (i.e. continue using the current HEAD). That would eliminate the need to update the MPAS CORE/develop branches immediately - at least until some new feature in MPAS-Tools is required.

I apologize for not reading the details of this PR carefully, but do you think it is possible to implement the new command line flags while still retaining backward compatibility with the existing argument usage?

akturner · 2016-11-23T15:23:20Z

@matthewhoffman No, its not possible with getopt to retain the original usage. I didnt realize this would create so many issues, when all I needed was to add a single functionality to one of the tools. If you want to roll back the previous commit and just carry on as the tools are feel free.

xylar · 2016-11-23T15:36:43Z

@akturner and @matthewhoffman, I'll let the two of you decide if we want to proceed with this PR. The syntax does seem better and more similar to other tools we use with this PR. But it is also a big pain to make these changes, since we lose backwards compatibility.

@akturner, it should be possible to add your one flag using @douglasjacobsen's old syntax instead of getopt, which would make the whole PR a lot easier to merge (if less clean).

matthewhoffman · 2016-11-23T15:46:33Z

@akturner & @xylar , I don't feel particularly strongly - the landice core is fairly stable at the moment, but I'm more worried about the ocean core where the ACME v1 finalization work has been a bit fragile.

As an aside, I had a thought that once we 'finalize' a place and name for the test case infrastructure, we could investigate adding MPAS-Tools as a git submodule within the testing infrastructure directory. Then versions of the test_case infrastructure could be tied to versions of MPAS, and it could also simplify where to find the individual tools and what their names are. For users who don't have access to the MPAS-Tools repo, the submodule would just not update (but we'd have to look into how that looks/operates). There are other complications, like how to get the individual tools in the submodule built without having to do in manually. So I'm not sure this a great idea, but give it some thought (unrelated to this PR).

In the meantime, I'm on the fence about what to do here. @mark-petersen , as another regular user of the test case infrastructure on the ocean side, do you have an opinion? We could wait and discuss this on Monday's telecon, depending on how urgent this is for @akturner.

akturner · 2016-11-23T15:52:49Z

@matthewhoffman This is not urgent at all. I think we should just roll everything back to how it was. I apologize for wasting everyones time.

xylar · 2016-11-23T16:07:21Z

I had a thought that once we 'finalize' a place and name for the test case infrastructure, we could investigate adding MPAS-Tools as a git submodule within the testing infrastructure directory. Then versions of the test_case infrastructure could be tied to versions of MPAS, and it could also simplify where to find the individual tools and what their names are. For users who don't have access to the MPAS-Tools repo, the submodule would just not update (but we'd have to look into how that looks/operates). There are other complications, like how to get the individual tools in the submodule built without having to do in manually. So I'm not sure this a great idea, but give it some thought (unrelated to this PR).

I think that's a great idea. Presumably MPAS versions could be released with associated versions of the tools, so there shouldn't be anyone that doesn't have permission to get the tools they would need to set up MPAS test cases. Presumably some tweaks to the makefiles (along the lines of #136) would make it easier to build the tools as part of setting up test cases. Most of the tools we use on the ocean side are python-based at this point anyway.

xylar · 2016-11-23T16:08:30Z

@akturner, are you sure you don't just want to change this PR to add the one flag you wanted in the first place? That way you at least get the index map you need.

akturner · 2016-11-23T16:09:20Z

@xylar I might do that later but for now lets just roll back and I'll submit a new PR later

xylar · 2016-11-23T22:07:22Z

There's not anything to roll back. You just close the PR and delete the branch (unless you want to keep it)

xylar · 2016-12-04T10:37:26Z

@akturner, should I go ahead and close this PR?

akturner · 2016-12-05T16:08:33Z

Yes please, Xylar. Thanks

xylar · 2016-12-05T20:09:45Z

@akturner, I also deleted the remote branch. You can restore it if you would rather keep it. But make sure you do so soon before it's gone for good...

akturner added 2 commits November 17, 2016 21:50

Removed trailing whitespace

cc4f7d0

akturner added the enhancement label Nov 18, 2016

akturner assigned mgduda, mark-petersen, xylar and matthewhoffman Nov 18, 2016

xylar reviewed Nov 18, 2016

View reviewed changes

Removed ability to read in filenames from stdin

defa21f

Removed the ability to read in filenames from stdin. The code now defaults to grid.nc for input and culled_mesh.nc for output. Corrected the default output to culled_mesh.nc Updated the usage statement.

xylar mentioned this pull request Nov 23, 2016

Add -i and -o flags to all mesh conversion calls in ocean test cases MPAS-Dev/MPAS#1148

Closed

xylar closed this Dec 5, 2016

xylar deleted the cellCuller/output_culler_cellmap branch December 5, 2016 20:09

Added option to write out the cellMap variable to a text file #149

Added option to write out the cellMap variable to a text file #149

Uh oh!

Conversation

akturner commented Nov 18, 2016

Uh oh!

xylar Nov 18, 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!

xylar Nov 18, 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!

xylar Nov 18, 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!

xylar commented Nov 18, 2016

Uh oh!

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

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xylar commented Nov 18, 2016

Uh oh!

akturner commented Nov 18, 2016

Uh oh!

akturner commented Nov 18, 2016

Uh oh!

xylar commented Nov 18, 2016

Uh oh!

xylar commented Nov 18, 2016

Uh oh!

xylar commented Nov 18, 2016

Uh oh!

mark-petersen commented Nov 18, 2016

Uh oh!

xylar commented Nov 18, 2016

Uh oh!

akturner commented Nov 18, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xylar commented Nov 18, 2016

Uh oh!

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

Uh oh!

Uh oh!

akturner commented Nov 22, 2016

Uh oh!

xylar commented Nov 22, 2016

Uh oh!

matthewhoffman commented Nov 22, 2016

Uh oh!

xylar commented Nov 22, 2016

xylar Nov 18, 2016 •

edited

Loading

xylar Nov 18, 2016 •

edited

Loading

xylar Nov 18, 2016 •

edited

Loading

xylar commented Nov 18, 2016 •

edited

Loading

akturner commented Nov 18, 2016 •

edited

Loading

xylar commented Nov 18, 2016 •

edited

Loading

xylar commented Nov 23, 2016 •

edited

Loading