search

oirlab / liger_iris_pipeline

Data Reduction System (DRS) for the Thirty Meter Telescope IRIS imager/spectrograph
https://oirlab.github.io/iris-pipeline
0 stars 3 forks source link

import datamodels for TMT from jwst #6

Closed zonca closed 5 years ago

zonca commented 5 years ago

Following recommendations https://github.com/spacetelescope/jwst/issues/3079#issuecomment-475931620 to decouple iris_pipeline from the jwst package.

zonca commented 5 years ago

Implemented using entry_points as outlined in https://github.com/spacetelescope/jwst/issues/3079#issuecomment-475931620, it works great, I'm impressed.

@jdavies-st could you please take a quick look at this and check I haven't imported too much code from jwst? I had to import model_base.py so I could override URL_PREFIX, is there a better way?

jdavies-st commented 5 years ago

Hi @zonca, sorry for the big delay getting back to you. My apologies.

I see you've essentially forked jwst.datamodels here. That's fine if that's your intention.

I do want to bring to your attention a backwards-incompatible change we are making to jwst.datamodels that changes our AsdfExtension subclass, specifically the url_mapping property.

https://github.com/spacetelescope/jwst/pull/3437

We had implemented it incorrectly in datamodels and are fixing it now. It means that if you want to access schemas from the jwst.datamodels package and you don't want to import jwst.datamodels, you'll need to use a new URL (see description in PR). Not sure if you are using entry_points to access our schemas. Probably not, but I just want you to be aware.

As for your implementation here, I suspect a cleaner implementation would for us to pull out datamodels as a separate package from jwst and clean out all the jwst-specific code in it. And then you could just import it and use it without having to fork it as you've done here.

We do have some motivation to this I believe, as we are planning on using this infrastructure for the WFIRST mission. But I don't know the priorities or timeline for this at the moment.

Of course jwst.datamodels is open source, so if our priorities don't align with yours, we are happy for contributions. =)

jdavies-st commented 5 years ago

You can always subclass DataModel or one of the DataModel subclasses, and in your subclass, define schema_url to point to your own local schema for TMDDataModel. In that schema, there's no need to include our core.schema or anything else. Or you can make your own TMT-appropriate core schema. There are some attributes that will probably have to be in there, but only a few.

zonca commented 5 years ago

yes, thanks, I implemented it at https://github.com/oirlab/iris_pipeline/pull/6/files#diff-8b34b232d61372787e5fcfc26b4f154fR81, I also created a tmt_core schema which has a lots of jwst stuff inside but I'll clean it later.

The only thing that concerns me is that I need to monkeypatch jwst.datamodels._defined_models to allow datamodels.open to work with my models, any workaround for this?

import jwst.datamodels
import iris_pipeline.datamodels                                                                                                                                                                                                                                            
for class_name in iris_pipeline.datamodels.__all__:
    jwst.datamodels._defined_models[class_name] = getattr(iris_pipeline.datamodels, class_name)