adding the maps subdirectory to the dustdir variable #119
Conversation
|
@moustakas: is this what you had in mind to fix #118? If so, ask @weaverba137 for a review prior to any merge. |
py/desiutil/dust.py
Outdated
| @@ -189,7 +190,8 @@ def __init__(self, mapdir=None, north="SFD_dust_4096_ngp.fits", | |||
| south="SFD_dust_4096_sgp.fits", scaling=1.): | |||
|
|
|||
| if mapdir is None: | |||
| mapdir = os.environ.get('DUST_DIR', '') | |||
| dustdir = os.environ.get('DUST_DIR', '') | |||
There was a problem hiding this comment.
So if DUST_DIR is not set, dustdir = '', and mapdir = '/maps'. I think it would be better to raise an exception saying that the maps can't be found, rather than wait until a somewhat obscure error is raised when looking for fits files in a directory that doesn't exist.
There was a problem hiding this comment.
Thanks for the rapid feedback. I've added some better error handling.
py/desiutil/dust.py
Outdated
| @@ -189,7 +190,8 @@ def __init__(self, mapdir=None, north="SFD_dust_4096_ngp.fits", | |||
| south="SFD_dust_4096_sgp.fits", scaling=1.): | |||
|
|
|||
| if mapdir is None: | |||
| mapdir = os.environ.get('DUST_DIR', '') | |||
| dustdir = os.environ.get('DUST_DIR', '') | |||
| mapdir = "{}/maps".format(dustdir) | |||
There was a problem hiding this comment.
Use mapdir = os.path.join(dustdir, 'maps').
There was a problem hiding this comment.
Yes, that's a better choice. Done.
…os.path.join to form directory name
|
Oops, @weaverba137 I just found a minor typo. Please hold off any further review. |
|
OK, @weaverba137, I'm done with my review response. Thanks. |
py/desiutil/dust.py
Outdated
| dustdir = os.environ.get('DUST_DIR') | ||
| if dustdir is None: | ||
| log.critical('Pass mapdir or set $DUST_DIR') | ||
| raise IOError |
There was a problem hiding this comment.
Typically when raising an exception, you pass a message to it. In this case, the same message as the log message will do.
Also IOError might not be ideal here because the exception hierarchy is different between 2 & 3. I recommend carefully reading https://docs.python.org/3/library/exceptions.html to see if that's what you really want. For example, ValueError might be just as appropriate.
Please also make sure that the unit tests actually test the case where an invalid value is passed during initialization.
There was a problem hiding this comment.
I now raise a ValueError with a message, and I've added a unit test to check that ValueErrors are thrown appropriately when DUST_DIR doesn't exist or when a meaningless map directory is passed.
|
Looks good. Merge when tests are done. I will tag after the merge. |
|
desiutil/1.9.13 is installed on cori and edison. |
This PR updates the default directory used by
desiutil.dustto add the/mapssubdirectory to the$DUST_DIRenvironment variable.