Skip to content

Conversation

xylar
Copy link
Collaborator

@xylar xylar commented Feb 12, 2018

This is the recommended python philosophy: "explicit is better than implicit".

@xylar xylar added the clean up label Feb 12, 2018
@xylar xylar self-assigned this Feb 12, 2018
@xylar xylar requested a review from jhkennedy February 12, 2018 19:45
@xylar
Copy link
Collaborator Author

xylar commented Feb 12, 2018

This follows from: #280 (comment)

@xylar
Copy link
Collaborator Author

xylar commented Feb 12, 2018

Testing

I ran the QU240 test case I always use on my laptop. Once I got all the absolute imports right, everything worked as expected.

@xylar
Copy link
Collaborator Author

xylar commented Feb 12, 2018

@jhkennedy, I would suggest two tests if you are willing to review this PR:

  1. run the analysis on one of the NERSC simulations we use, which entails:
    a. copy a config file in configs/edison/config.* .
    b. edit the config file to give a output directory (typically on $SCRATCH or $CSCRATCH, but you can't use an environment variable name).
    c. run the analysis: ./run_mpas_analysis --purge config.<run_name>. (The --purge flag is optional but I find it useful in case you need to run more than once or you want to reuse the same scratch location as you've used in the past.)
  2. do some kind of a search through the code to make sure I didn't miss any relative imports. I think I got them, but a second set of eyes would be useful.

Thanks!

@jhkennedy
Copy link

@xylar Happy to! I'll put it on my schedule for tomorrow.

Copy link

@jhkennedy jhkennedy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Everything, except two very small changes, looks good to me here!

Unfortunately, I'm not a member of the acme group on Edison, so I had to run the tests on Titan. Other than a few errors from missing files (which look correct and not caused by this PR) everything ran smoothly and output the expected website.

The changes are just small formatting changes -- there are a couple of from import statements that have two spaces.

from .utility import paths
from .write_netcdf import write_netcdf
from .mpas_reader import open_mpas_dataset, subset_variables
from mpas_analysis.shared.io.namelist_streams_interface import NameList, \

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are two spaces here after the from

@@ -1 +1 @@
from .remapper import Remapper
from mpas_analysis.shared.interpolation.remapper import Remapper

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are two spaces here after the from

This is the recommended python standard.
@xylar xylar force-pushed the remove_relative_imports branch from e994934 to 87ff980 Compare February 14, 2018 22:16
@xylar
Copy link
Collaborator Author

xylar commented Feb 14, 2018

@jhkennedy, I’ve fixed the two formatting issues you found (thanks!). Let me know if there’s anything else before I merge.

@jhkennedy
Copy link

Looks good to me; merge on!

@xylar xylar merged commit 209c76f into MPAS-Dev:develop Feb 15, 2018
@xylar xylar deleted the remove_relative_imports branch February 15, 2018 20:51
@xylar
Copy link
Collaborator Author

xylar commented Feb 15, 2018

Thanks, @jhkennedy.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants