search

ESCOMP / CISM

Community Ice Sheet Model
GNU Lesser General Public License v3.0
6 stars 11 forks source link

Move shared python code to a shared directory #18

Open billsacks opened 6 years ago

billsacks commented 6 years ago

I'm moving this old PR from https://github.com/NCAR/cism-development/pull/5 to this new repository.

Original comment from me, Aug 17, 2017

This demonstrates what I was thinking in terms of moving some duplicated python code to some shared location. I have implemented these changes for the dome test case. If you like this approach, the same changes would be needed in the other test cases, as well as tests/new/. Those changes should be made before merging this PR: I'm mainly opening this PR as a way to demonstrate what I was thinking.

I haven't given much thought to the naming of directories (i.e., 'lib/cism_tests_lib'), so feel free to suggest something else if you prefer.

There may be some other duplicated code that could also be moved: I haven't spent much time looking at what's in common between the different test cases.

billsacks commented 6 years ago

Comment from @jhkennedy Dec 1, 2017

Hey @billsacks I think this is a great idea and a ton of duplicate code could be removed.

I might suggest that it might be worth restructuring the directory entirely so that all the run*.py scripts (and any other scripts you'd like to take advantage of the shared code) are directly under $CISM/tests/. That way, you can avoid having to adjust the python paths:

https://github.com/NCAR/cism-development/blob/3bd87aacb1987e155cb49ad628d6b19885c3ccc7/tests/higher-order/dome/runDome.py#L16-L19


_TESTS_LIB_DIR = os.path.join(os.path.dirname(os.path.abspath(__file__)), "..", "..", "lib")
sys.path.append(_TESTS_LIB_DIR)

If you run a couple of these, your going to have that _TESTS_LIB_DIR in the python path many times (slowing down the python search and import process). Alternatively, you could just make sure it's not already in the path before appending it.

Most of the different test directories just contain config and data files, so they could go under a data/ directory and all shared code under a lib/ or utils/ directory to keep that top level cleaner.

This also would be a good time to ensure everything here is python 2 and 3 compatible.

I'm happy to help you with all this if you like.

billsacks commented 6 years ago

Comment from me Dec 1, 2017

@jhkennedy thanks for your thoughts on this. I opened this largely to share my thoughts via a prototype. I wouldn't have time to bring this to completion for a while. So if you have the time and interest, I'd love your help.

I hadn't realized the potential issues with adding to sys.path. I remember feeling like this was an ugly solution - I just couldn't come up with anything better at the time. I'd happily support a solution that makes this cleaner.