DM-49453 : DRP solar system ephemeris generation #1171
Conversation
|
rebase your commits to have approximately meaningful commits (i.e. not something like "jakes changes") |
| innerTractSkyRegion, | ||
| skyInfo.wcs) | ||
| associatedSsSources = associatedSsSources[ssInTractPatch] | ||
| print('masked', len(ssInTractPatch), 'ssSources to', sum(ssInTractPatch), 'in tract-patch') |
There was a problem hiding this comment.
No print messages. IF it is important, use the logger
| from lsst.pipe.base import connectionTypes, NoWorkFound, PipelineTask, \ | ||
| PipelineTaskConfig, PipelineTaskConnections, Struct | ||
| import lsst.pipe.tasks | ||
| from lsst.utils.timer import timeMethod |
There was a problem hiding this comment.
You have a mix off all different imports, direct symbols, import as, and bare import. Please prefer directly importing the symbols you need, i.e from lsst.pex.config import Field,...
| ]: | ||
| self.log.info(f'writing {filename}') | ||
| open(cache + filename, 'wb').write(fileref.fileContents.read()) | ||
| meta_kernel_text = f"""\\begindata |
There was a problem hiding this comment.
don't break up lines like this to avoid space use text wrap.dedent
| inputVisits.to_sql('observations', conn, if_exists='replace', index=False) | ||
| conn.close() | ||
|
|
||
| open(f'{tmpdirname}/eph.ini', 'w').write(eph_str) |
| ('naif0012.tls', naif0012), | ||
| ]: | ||
| self.log.info(f'writing {filename}') | ||
| open(cache + filename, 'wb').write(fileref.fileContents.read()) |
| \\begintext | ||
| """ | ||
| open(f'{tmpdirname}/sorcha_cache/meta_kernel.txt', 'w').write(meta_kernel_text) |
| open(f'{tmpdirname}/sorcha_cache/meta_kernel.txt', 'w').write(meta_kernel_text) | ||
| self.log.info('Sorcha process begun') | ||
|
|
||
| result = subprocess.run( |
There was a problem hiding this comment.
It looks like sorcha is written in python. Do not call the command line through subprocess, you should be making api calls against the code base.
There was a problem hiding this comment.
I can make this change but would really prefer not to. Sorcha is written in python, but only designed to be run from the command line. I could make api calls to the base code, but would have to construct a SorchaConfig and SorchaArgs object, both of which are opaque and were not designed as the normal interface by the Sorcha team.
If there's a hard rule against command line / subprocess, I can move to in-python Sorcha calls, but I think it would make the code less readable and actually break Sorcha's intended use.
There was a problem hiding this comment.
I've heard a bit more from pipelines folks about why child processes are unsupported in pipetasks. Might be worth a quick zoom chat to discuss options.
| # Format DRP ephemerides. Needs: | ||
| # ra | ||
| # dec | ||
| # ssObjectId |
There was a problem hiding this comment.
These comments seem to not refer to anything, what do they go to?
| fileContents = None | ||
|
|
||
| def __init__(self, fileContents): | ||
| self.fileContents = fileContents |
There was a problem hiding this comment.
I am not 100% reading through the code. I can't tell if you are using binary as a passthrough here and the data is actually stored in a readable way, or if you are indeed writing straight binary to disk. If it is the later we generally discourage that. Is this actually just passthrough to a csv or something?
There was a problem hiding this comment.
With a couple exceptions, the underlying data is unreadable binary stuff that we absolutely need to have as a file somewhere. Happy to hear about a better way to write a bunch of heterogenous binary files than open( , 'wb').write(data) but I'm not aware of one.
Gerenjie
force-pushed
the
tickets/DM-49453
branch
2 times, most recently
from
05d85c3 to
c30d792
Compare
Gerenjie
force-pushed
the
tickets/DM-49453
branch
from
c30d792 to
17f35bc
Compare
Gerenjie
force-pushed
the
tickets/DM-49453
branch
from
17f35bc to
201bfd4
Compare
Add generateEphemerides task and SSPAuxiliaryFile storage class.