geminidrsoftware / dragons Goto Github PK

The detector_name() descriptor for GMOS should incorporate information from the DETECTOR keyword. For example, for Hamamatsu GMOS-S, when the video board was changed, the CCDs remained the same and only the DETECTOR keyword can differentiate between post and pre. See the table below for examples.

Warning: Changing detector_name() would have implications for the GMOS lookup tables in gemini_instruments. So we probably want to discuss this a bit to avoid a mess.

Not urgent. This is not causing issues right now as it appears that changing the video board did not affect gains, etc. It just seems like we should be able to distinguish between changes in instrument hardware with the potential of affecting DR.

  FILE                       detector_name()               detector_name(pretty)     DETECTOR (keyword)

N20030630S0059.fits:   EEV9273-16-03EEV9273-20-04EEV9273-20-03 | EEV     |   GMOS + Red1
N20040423S0015.fits:   EEV9273-16-03EEV9273-20-04EEV9273-20-03 | EEV     |   GMOS + Red1
N20050423S0198.fits:   EEV9273-16-03EEV9273-20-04EEV9273-20-03 | EEV     |   GMOS + Red1
N20100701S0346.fits:   EEV9273-16-03EEV9273-20-04EEV9273-20-03 | EEV     |   GMOS + Red1
N20120401S0203.fits:   e2v 10031-23-05,10031-01-03,10031-18-04 | e2vDD   |   GMOS + e2v DD CCD42-90
N20170418S0090.fits:   BI13-20-4k-1,BI12-09-4k-2,BI13-18-4k-2 | Hamamatsu-N |   GMOS-N + Hamamatsu
S20030119S0039.fits:   EEV8056-20-03EEV8194-19-04EEV8261-07-04 | EEV     |   GMOS + Blue1
S20040715S0037.fits:   EEV2037-06-03EEV8194-19-04EEV8261-07-04 | EEV     |   GMOS + Blue1 + new CCD1
S20070630S0108.fits:   EEV2037-06-03EEV8194-19-04EEV8261-07-04 | EEV     |   GMOS + Blue1 + new CCD1
S20080630S0054.fits:   EEV2037-06-03EEV8194-19-04EEV8261-07-04 | EEV     |   GMOS + Blue1 + new CCD1
S20100630S0083.fits:   EEV2037-06-03EEV8194-19-04EEV8261-07-04 | EEV     |   GMOS + Blue1 + new CCD1
S20140623S0133.fits:   BI5-36-4k-2,BI11-33-4k-1,BI12-34-4k-1 | Hamamatsu-S |   GMOS + Hamamatsu
S20140903S0141.fits:   BI5-36-4k-2,BI11-33-4k-1,BI12-34-4k-1 | Hamamatsu-S |   GMOS + Hamamatsu
S20170620S0057.fits:   BI5-36-4k-2,BI11-33-4k-1,BI12-34-4k-1 | Hamamatsu-S |   GMOS + Hamamatsu_new
S20170620S0133.fits:   BI5-36-4k-2,BI11-33-4k-1,BI12-34-4k-1 | Hamamatsu-S |   GMOS + Hamamatsu_new 

When the point recalcuation panel gets large with fit1d, it looks busy/odd and we want to try a layout with the plot in the center. This would be

( recalc inputs. [[Tabs: ( plot, per-tab inputs) ]] )

The RangeField class accepts a min and a max parameter. When we run a primitive with reduce -r primitiveCheese -p param=[max], this primitive fails with a FieldValidationError. For example:

$ reduce -r traceApertures N20180508S0021_mosaic.fits -p max_shift=0.1 
   ... 
   ERROR - gempy.library.config.config.FieldValidationError: RangeField 'max_shift' (Maximum shift per pixel in line position
   ERROR - 	Valid Range = [0.001,0.1)) failed validation: 0.1 is outside of valid range [0.001,0.1)
   ERROR - 
   ERROR - Reduce instance aborted. 

We should reflect this behavior when creating Slides for Interactive Mode.

I've verified this in one of my test cases for calculateSensitivity. This seems to be inside the fit calculation itself and not the UI. Still investigating.

The interactive traceApertures() primitive is supposed to accept Key Mapping commands. Right now, all the commands displayed in the mini-help panel on the bottom right are ignored.

Spaces and commas should be interchangeable/mergeable. Also if the input is bad there should be some sort if alert or affordance to alert the user the input is bad

From the interactive summit:

In fitter app, with regions. We can do “r”-”r” to set a region. It would be good if we could do “e”-”e” to reset an edge instead of “e”-”r”.

In recipe_system.utils.reduce_utils.normalize_ucals, at line 380, a couple of sanity tests are performed. The first of these currently assumes that the tag set for a processed calibrator matches the calibrator name; e.g., it assumes that a 'processed_bias' will have tags ['PROCESSED', 'BIAS'] as part of its tag set.

This assumption breaks down for GHOST in two places:
'processed_slit' should have tags ['PROCESSED', 'SLIT*V]
'processed_slitflat' should have tags ['PROCESSED', 'SLITV', 'FLAT']

normalize_ucals (which is needed to use the Calibration Manager API) currently fails on at least 'processed_slitflat' because of this. I've temporarily solved the problem by simply commenting the test out, which isn't too dire (all the tests do is stop you making really silly mistakes when you're assigning calibrations programatically). However, it probably needs adjusting to actually read in the tag associations for each calibration type and instrument (and I can't recall where those live).

I've started checking our GHOSTDR development (https://github.com/ANU-RSAA/GHOSTDR) against Python 3.5, given there are Python 3+ syntax and dependencies now within DRAGONS.

I've been doing this by running our test suite in an environment similar to the original Python 2.7 geminidev environment that our install instructions required, by updated to Python 3.5.6 (and with relevant other package updates). I'm currently getting the following error, having run the cythonize command within DRAGONS/gempy/library as instructed:

ImportError while importing test module '/Users/marc/Documents/ghost/git/GHOSTDR/ghostdr/ghost/test/test_ghost_spect.py'.
Hint: make sure your test modules/packages have valid Python names.
Traceback:
ghostdr/ghost/test/test_ghost_spect.py:32: in <module>
    from ghostdr.ghost.primitives_ghost_spect import GHOSTSpect
ghostdr/ghost/primitives_ghost_spect.py:31: in <module>
    from gempy.mosaic.mosaicAD import MosaicAD
E   ImportError: bad magic number in 'gempy.mosaic': b'\x03\xf3\r\n'

Given mosiacAD doesn't appear to be in the repository, it appears to be created by the cythonize command instead.

The information about the current filename needs to be bigger or more prominent, somehow. We have not decided how to do that. We probably need to try a few things. See the current state below:

When a user wants to change the behavior of reduce at some point, (s)he would have to know: 1) the recipe name, 2) the primitive name, 3) the parameters for that primitive. We don't have that yet. So I am planning to dig into the code and create two new scripts that will perform (1) and (2). showpars already solve the (3) problem but not if we a running a recipe different from the default. So here is my plan:

• Create show_recipes to display the recipes available for a given file.

• Create show_primitives to display the primitives available for a given recipe/file.

• Update showpars to list the parameters related to a primitive that belongs to a no-default recipe.

When the system attempts to throw the IOError on line 526 of geminidr/core/primitives_standardize, it itself throws a new Error, because the number of values in the .format call is less than the number of replace positions in the string. Suggested fix:

raise IOError("The {} extension(s) in {} does not match the " "number of extensions expected in raw {} " "data.".format(len(ad), ad.filename, inst_name))

While dealing with the #112 and #113 issues, I tried to find documentation about the python versions that we support in DRAGONS but I could not find anything. I believe that this kind of information should be described in the README.md file and in the DRAGONS documentation webpage. I believe that now would be a good time to updated this kind of documentation considering that @jehturner is going again through the process of reviewing the required package. I can help with that but I would like to know the team's opinion before writing anything.

Right now, the interactive traceApertures() Web UI has a maximum width which is set via CSS. The plots' height also reaches a maximum and does not scale with the window's size, leaving lots of blank space. We want the plot's size to scale with the window to maximize its use.

Laundry list of unifying style changes inspired by Bruno's work:

• thinner top banner
• use only primitive name in camelcase for title
• use font from Bruno's updated UI
• smaller DRAGONS icon
• padding before title

To mask individual points, right now, one has to click and press a key, actually one also has to remember to select the appropriate bokeh tool. If possible, it would be preferable to be able to just point and press a key. (even if there is still a need to select the appropriate bokeh tool.)

Mask/Unmask help is confusing if no selection has been made yet.

I want to refactor the Controller help text handling to dynamically respond to selection. Apertures/Regions should likewise be able to change their section of the help when they are in different states. Also will unify on Bruno's styling while making these edits.

Based on comments on the last SUSD C&C meeting, I believe that I should add some documentation about tests and the existing Continuous Integration tools for DRAGONS within the repository.

It is important to note that we have two CI tools: GitHub Actions (public) and Jenkins (internal). Documentation on Jenkins should be only available for NOIR's Lab / GEMINI staff since it might contain sensitive information.

The format of this documentation should be in Sphinx/RST since it is the default documentation framework used in DRAGONS.

Instead of having the column of start/stop aperture edit fields for all apertures, have a concept of a single aperture selected for editing and only show the values for that one. This would integrate nicely with my 'S' to select method and color highlighting of a selected aperture in the figure.

The ETI logic sets up a subprocess and queue pair. If the subprocess fails for an unexpected reason, the calling code never realizes it will be waiting forever on the queue.

I propose we handle any exception in running the subprocess.

A recent test for QECorrect started to fail. Chris changed the parameters but the reason it is failing is that the test is not performing as expected. See plot below:

You can see that the data seems fine, but the functions used to adjust the spectra diverge on the second gap due to the existence of a strong absorption line near the edges.

The sigma mask should reflect inputs that were masked from previous iterations, and not also include points that are sigma rejected off the fit but were included in it's calculation.

I need to add a concept of a "current/selected" aperture and color code it in the figure.

• Delete Aperture will operate on selected aperture
• An aperture being moved or adjusted will be selected while the adjustment is under way
• When the adjustment is completed/set, revert to no active aperture
• Need a "Select" option to select an aperture without initiating a resize or delete - so the user can select from multiple apertures at the same location for a delete

See if we can have the cursor symbol change in response to the operation being performed at the time.

Bruno did a refactor for traceApertures - it makes sense to do it more broadly to clean up the top level html

The base interactive framework should make it easy for UIs to provide custom help. The help should be launched via Bruno's button in the title bar and use Simon's popup window style interface.

Tests for p.findSourceApertures() are now failing. The traceback is attached.
traceback.txt

When using addReferenceCatalog with the Vizier catalogs, there are two issues:

• 2mass_vizier returns nothing. It seems to be a problem with the url (http://vizier.u-strasbg.fr/viz-bin/votable/-A?-source=B/2mass&). Using http://vizier.u-strasbg.fr/viz-bin/votable/-A?-source=2mass& instead works (without the B/), or http://vizier.u-strasbg.fr/viz-bin/votable/-A?-source=II/246& (with the explicit catalog name).

• sdss9_vizier: RAJ2000 and DEJ2000 are not in the returned columns. The catalog contains RA_ICRS and DE_ICRS instead, but Vizier also provides _RAJ2000 and _DEJ2000 so we could use those.

The traceApertures primitive raises IndexError: index 0 is out of bounds for axis 0 with size 0. errors when running interactively depending on the Tracing Panel's parameters (left panel).

csimpson 5:06 PM
I think a useful thing to add to the interactive plots would be the number of "fittable" points (i.e., not rejected by the user either individually or from regions) and the numbers of good and rejected points

Selection masking is discussed in the controller help (M/U/P key explanation) but that should not appear when selection masking is not available.

I had to run some tests locally to verify some regression tests. I didn't remember that Py37 is now a requirement for DRAGONS so I ran the tests using one of the py36 environments. I got the following error:

traceback.txt

My first question is if DRAGONS should run with Py36 or not. I remember some discussions but I don't remember the outcome. If yes, we have to find a solution to the error above. If not, we have to remove py36 from tox.

This may be a max value for the parameter not being propogated/handled on text entry. I should rollback if so.

We also need some sort of indication to the user, such as field validation, that their input is invalid.

In the “fitter apps”, the label “sigma” for the points rejected based on sigma clipping is very confusing. It really needs a new name. “sigma clipped”? “rejected (sigma)”? Other ideas? The “rejected (sigma)” might work well with “rejected (user)” for user rejected points.

Two issues confirmed with Chris:

The residuals calc before sigma clip is inverted, which is a problem when sigma upper/lower differ
sigma upper/lower should be allowed to be 0.0, rather than cascading to sigma per default

Kathleen had a case where it takes some time with the recalc, I should activate the modal message until it finishes.

> ad2 = copy(ad)
Traceback (innermost last):
  File "<console>", line 1, in <module>
  File "/home/jturner/anaconda3_2019.10/envs/spec37/lib/python3.7/copy.py", line 106, in copy
    return _reconstruct(x, None, *rv)
  File "/home/jturner/anaconda3_2019.10/envs/spec37/lib/python3.7/copy.py", line 282, in _reconstruct
    if hasattr(y, '__setstate__'):
  File "/home/jturner/repos/DRAGONS/astrodata/core.py", line 532, in __getattr__
    if self.is_single and attribute.isupper():
  File "/home/jturner/repos/DRAGONS/astrodata/core.py", line 532, in __getattr__
    if self.is_single and attribute.isupper():
  File "/home/jturner/repos/DRAGONS/astrodata/core.py", line 532, in __getattr__
    if self.is_single and attribute.isupper():
  [Previous line repeated 489 more times]
RecursionError: maximum recursion depth exceeded

@chris-simpson notes that "it looks like the problem is that the AD getattr has the line if self.is_single and so this falls into a recursive loop if is_single isn't defined when getattr is first called" and "I suspect that it fairly easy to have it work correctly by defining the __copy__() method [TBC]".

The recent issue #112 raised back the problem of which Python versions are supported by DRAGONS. Up to very recently, I remember that Py36 was being supported but is this still true? And what about future python versions? We are not testing them. The tests only run on Py37.

The Unit Tests in Jenkins could run several Python versions since they are short. That would help to verify the installation and potential problems. Very recently, we removed the parallel {} clause from the Unit Tests stage. I suggest adding this clause again to the Jenkinsfile with test cases where we have tests for different python versions.

Decision from the interactive summit, add regions to the interactive mode for normalizeFlat.

The upper and lower edge of the apertures are shown/editable as absolute coordinates. These should just be a distance from the aperture location i nstead.

The interactive traceApertures() primitive contains a box below the plots that we can use to select regions of data used for fitting. This box is now completely ignored. It has no functionality.

Sometimes, the Y-axis labels have way too many zeros. See the example below:

If we have more than 2 zeroes, the y-axis labels should be displayed in scientific notation.

It's unclear whether DRAGONS is accepting contributions. Assuming that the DRAGONs team wishes to receive them, having a CONTRIBUTING.md will encourage other OSS developers in the astronomy field to contribute.

Here are just a few public examples of good CONTRIBUTING.md and with "How-To" instructions:

1. https://gist.github.com/PurpleBooth/b24679402957c63ec426
2. Auth0's: https://github.com/auth0/open-source-template/blob/master/GENERAL-CONTRIBUTING.md
3. https://github.com/nayafia/contributing-template/blob/master/CONTRIBUTING-template.md

Instead, use a popup alert to notify the user when they try and exceed the bounds hard limits

Currently, the 1-d fitter always makes a column down the left if there are more recalc driving inputs than the fitting function. Instead, I want to make it an option for the constructor so each specific primitive can opt for a flow layout of the inputs above the tabs.

This may or may not be an appealing alternative, so this is POC/feedback pending.