Report on MIRI Code Review from 11/24/08
Covering work done on the XML Schemas (Pat McCauley) and Java Templates (Rob Douglas).
Attendees:
Testers
Karla Peterson
Kate Henshaw
Chris Ritchie
Commandos
Ilana Dashevky
Perry Rose
Mike Robinson
Developers
Andy Spina
Tom Donaldson
Pat McCauley (Reviewee/Facilitator)
Rob (Facilitator/Reviewee)
XML Schemas and Save and Load
* Can you save and reload an alphabetical string into a Double Field? Should we allow it in JWST?
p. 14.
We held a discussion with the developers:
Fields that are numeric should still be allowed to be saved even if they are not "correct". So we decided that all these fields should be Elements and have an attribute StringVal that can be used to store the String if the value is not parsable. Make a method to convertFromJAXB, convertToJAXB on ConstrainedDouble, Dates, etc....
* ScientificKeyword should be ScienceKeyword, p. 16
We will change this.
* Can we get rid of Retrieval method from the Prop Inst? p.16 - this is not in Propinst - remove from Schema
Remove retrieval method
* When generating Visits - what goes in XML, and what is regenerated
We deferred this issue to deal with later.
* What about the namespaces for mit: vs mdark
We decided to refactor the Schema to include a shared MIRI schema for shared concepts
across templates.
* SHould we allow all Filters to include all the Filters (instrument level)? p. 18
We decided the Schema should not enforce template rules, such as limiting the allowed
values from an enumeration. Any filter should be allowed.
* HOw do we cleanup unused XML schema structures
This will be left to the developers to identify and remove them. There is no automated way.
* Dithering should be added to all schemas
We will add Dithering to the schemas.
* SHould we be storing CalculatedEXpTime?
This is being deferred until a need comes up for it.
* Should make derived field names the same in all templates: MiriImagaing Groups, Integrations -> LRS NumberOfGroups.... ets.
This will happen with the MIRI shared Schema
* MiriCoron - exposure definition is duplicated at top level - needs removed (including Filter....)
This will be removed
* MiriCoron - should XML be saving what the user put in? Combo field, or just the one
Since the Mask and Filter are derived, we will save the combo field to the XML.
* Remove Filter from MiriDark
It will be removed
* Should FiltersConfig have the word exposure instead of filter
We decided to rename the element ExpConfig. In a recent discussion, we now
use a new MIRI_Exposure_Specification table for storing parameters, so we
might make this ExpSpec.
* MiriImagingFlat - need to support all the Filters - e.g. FND
Again, we will not have the XML do the enforcement of what is allowed.
* Why is ALL allowed in MiriDark?
Again, we will not have the XML do the enforcement of what is allowed.
* MiriMRSFlat - should we include PRINCIPAL in the list of enum?
Again, we will not have the XML do the enforcement of what is allowed.
* MiriAnneal ImagerType should be DetectorType [ACTION]
This will be changed
(A quick note - in general, I believe the XML should allow the saving and loading of anything that could be input into APT. If a user puts something in the GUI and then has to rush off to a meeting and wants to save and quit, the values in the GUI should not prevent that - even if they are technically illegal. I also believe that enums should allow any of the enum values, and leave the DM code to set legal values - that will allow things to be later made legal or illegal in the template, without having to change the schema.)
Template Code
• Anchored Note, page 32
For later discussion...
Tom's concerned about a lot of code happening in Observation's unnamed initialization - he finds it hard to read and would prefer a "setupProperties" method
We decided that we would create an infrastructure to manage all these initializations. Cosi would be able to use annotations to identify constraints, and all Calculators and other anonymous class uses would be instead turned into named inner classes to encapsulate the logic, in the case of Derived Properties.
• Anchored Note, page 34
Question about key selection
Andy had a question about key selection, Rob punted.
This will be reviewed when we do more Visit Creation
• Anchored Note, page 37
How to hide engineering modes
Karla asked how we would hide engineering modes
We will discuss that when that behavior is added later
• Anchored Note, page 37
Don't use newInstance
Andy suggested writing a newInstance method rather than using the reflective "newInstance()" method
Andy will tell Rob about the scheme he had in mind
• Anchored Note, page 40
FGS is not an instrument
Christine asked why FGS was not an instrument. Rob said FGS is not really an instrument.
FGS will come with its own templates, but the science instrument is called TFI.
When rules come for FGS Engineering Templates, we will add that Instrument.
• Anchored Note, page 40
The "name" should be final
Andy says the name should be final. name field should be private instead of protected as well.
Agreed - we will make it so. We will also make other Fields final for most templates
• Anchored Note, page 40
What's the purpose of this class
Tom asked why this class is called JwstInstrument. He doesn't understand the need to have this class. He was confused why the class had a static list of subclasses. Why didn't we make this an enum? Rob will dig up notes.
We discussed this and decided to keep the Instrument class in the hierarchy, though it does not
know about its decendents. It will just house shared instrument information. A new JwstObservatory
will keep track of the individual Instruments.
• Anchored Note, page 40
Tom wants to move the "getInstruments" method
The list made it confusing for Tom.
Moved to JwstObservatory
• Anchored Note, page 41
Calculated should be Actual
Christine pointed out the Prop Inst states "Calculated Exposure Time" should be "Actual Exposure Time"
General Note: Need to clean up GUI and schemas to ACCURATELY reflect Prop Inst.
We will do this.
• Anchored Note, page 41
philosophy question about instrument
Tom wanted to know what the MIRI Instrument DOES - since we have a bunch of static methods, can we pull out the TINA stuff from these classes? In other words, the Instrument classes shouldn't know anything about TINA.
Maybe the construction of these fields would be better left to the document model, just provide query methods on this class.
Rob responded: just wait, you'll see. He said it's partially to reduce code duplication - since most templates use a Readout Pattern table, we only have a single place where we construct a field.
Andy raised the question of whether we should create a MIRI Field Factory.
We moved the Field creation methods to a new MiriTemplateFieldFactory. The Instrument
contains all the enums for the allowed values of most fields, and will provide static access
to any instrument information needed.
• Anchored Note, page 41
Should we label these "Exposure"?
Karla asked if we should consider calling "Filters" "Exposures", especially in the Miri Dark template
We will change the name for the Dark to be Exposure, but others will remain Filter.
• Anchored Note, page 44
Get rid of "TODO" comment
Rob's note to self - on MiriDetector enum
This will be done
• Anchored Note, page 45
What are these numbers?
Christine asked what the numbers were for MiriFilter and why they were used. Is 1000 really the right number for maxInts? Need to change this when prop inst gets updated.
They come from the Propinst, but are not used. We will leave them there and see if
they become useful.
• Anchored Note, page 49
These should be private FINAL
And this applies to every other template
Referring to the Template Fields - agreed, they should all be private or protected as needed, and final.
• Anchored Note, page 50
When is this used?
Tom asked about newExposure vs. addExposure - Rob answered that newExposure is when user hits the "New..." button
Tom suggested we change "iRow" in addExposure method. He also didn't know where newExposure was called because there are no references to it.
We will clean the code to only include what is used. In this case, only newExposure is used.
• Anchored Note, page 50
Do we want to override createVisit before we have a need for it?
No. We decided to use JwstVisit until an override is needed.
• Anchored Note, page 51
Organizational Note
Constructor should not be above private fields.
All the code will be reorganized to follow a standard format
• Anchored Note, page 51
Setting several fields?
Tom's concerned the constraint is unnamed - doesn't have a Java name, but has a debugging name. However, the name doesn't encapsulate everything this constraint does.
Should we break out this constraint into multiple constraints? Should we rename it? Rob wants to do it as an atomic action, so don't split them out.
This is to be fixed with several changes. No anonymous classes, a better name for these actions,
and moving the whole thing to a single method, which can then be called by the Constraint, so
it is clear the collection is an atomic action.
• Anchored Note, page 51
Karla's confused
There is exposure-specific knowledge in the LRS template, but not in the MIRI Imaging Template.
This was reorganized to move the Exposure data to a separate class, and then share
inheritance of that information in the templates, or in the Exposure tables for templates
with multiple exposures.
• Anchored Note, page 52
What's the difference between attributes and properties?
Tom asked what the difference was between getAttributes and getProperties
Rob responded "getAttributes" return the properties for a Template - getProperties returns all properties on a TDE - maybe call these "getTemplateProperties"
All this was renamed getTemplateProperties and a default implementation was added.
Now templates need only override their templateProperties lists.
• Anchored Note, page 54
Question about ACQ stuff
This is different from another template - no TAcq flux. Rob had an action item to figure out why.
For MIRI MRS Target Acq, no target flux is necessary (according to Perry)
This will be handled by getting the flux from the Target. No specific change is needed now.
• Anchored Note, page 55
LRS and MRS are very similar
Should we move some of this into a shared class?
We decided to make this a shared class.
• Anchored Note, page 57
Not a complete initialization
Rob didn't complete initialization on this - should this be a convention, or only when needed?
We discussed how to make initialization less vulnerable to developer error, and didn't
come up with a good way to do it. But we did get rid of Delayed constraints in our reworking
of the Cosi. So we are in principal fine with this.
• Anchored Note, page 58
This uses same KEY for different visits
This will be a problem.
This is deferred until we do more visit creation
• Anchored Note, page 62
Should explicitly default filter
Filter should default to NULL and be disabled
Filter defaulted
• Anchored Note, page 62
Tom's general code comment
If something is not immediately obvious from reading the code, it deserves a comment. FlatSuite.get doesn't help, so a comment would be nice.
Need to add comments in those places. Code will be examined and comments added
• Anchored Note, page 64
lLegals assignment line should be removed
This was removed
• Anchored Note, page 67
This class doesn't complete its constraints either
Same issue as above.
• Anchored Note, page 67
This code is wrong
According to Rob, this code is wrong
This code was removed and done a different way
• Anchored Note, page 67
FND may be missing from the XML schema
Schema will not limit the Filter names, so this won't be an issue
• Anchored Note, page 67
Tom's thought about ATDE
Would it be worthwhile to have an extension of ATDE for Template DEs?
We decided this was not worth pursuing, but it was handled with other
refactoring.
• Anchored Note, page 70
Total Exposure Time may not be ever needed (no requirement for it)
I will remove this code
• Anchored Note, page 70
AbstractTinaMultiField - should we use something else?
Andy suggested we may want to use something else when Gary gets his code in.
As of now, this is the right way to do this work. It could be refactored later as needed.
• Anchored Note, page 72
Maybe we shouldn't be doing this parsing here
I will remove this Parsing.
• Anchored Note, page 72
Maintenance problem
Tom wants to know how we can minimize the maintenance of this. Maybe create an array of arrays? Array of objects, or something?
I will file a separate PR to change the way this is done - to make it more maintainable.
For now, it works and will be left as is. (PR 61457)
• Anchored Note, page 74
Should Miri Coron be called Miri Coron Imaging?
Should make this uniform across the board - commanding doesn't seem to care
We decided to just call it Coron for method and class names. The official name is
still MIRI Coronagraphic Imaging.
• Anchored Note, page 78
This needs a comment!
This class was completely removed and subsumed. Comments will be added for
anything else that is not clear as per points above.
• Anchored Note, page 78
Not another ETC!!!!! - call it something else
We renamed the MiriExposureTimeCalculator to MiriExpDerivedParamsCalculator
• Anchored Note, page 78
Do we really know at this level if we're in commanding mode?
We may want to consider who we're asking whether we're in commanding mode.
We decided that the functionality this addressed was needed, even though this is
not the ultimate implementation of it. It will be fine for now after the other refactoring.
• Anchored Note, page 79
Consider sharing this at the MIRI level
This way we won't have code duplication
We have moved all exptime and other derived property calculations to be handled in
one way, avoiding this duplication.
• Anchored Note, page 81
Code overstepping its bounds
Andy says setReadoutPattern should just take a string
General note: Remove deprecated methods.
Deperecated methods will be removed from the JWST code.
• Anchored Note, page 81
This makes Tom nervous
Me too. We want to consider some other way of doing this - null parent could be bad.
This is how the same thing is done currently in APT for similar fields. Other
work may make this obsolete, at which time it will be removed.
• Anchored Note, page 84
Why is this different?
Sometimes we define all legal values. Sometimes we remove some of the legal values.
A preference was to have the legal values defined, so if a new value is added it does
not become legal by default. Other instances will change to match this.
• Anchored Note, page 84
Confusion on what these methods are for
Tom was confused on how/why these methods are used. We have a "get<something>Field" and a "get<something>"
These fields were removed.
• Anchored Note, page 86
Use "getCoronFilter" rather than fCoronFilter.get()
Will do that.
• Anchored Note, page 89
Question to Rob
Why did he do number of exposures this way?
This was changed to match the standard way of doing Fields in templates
• Anchored Note, page 90
Add number of exposures to toString
From Tom: Also, consider adding something that indicates what kind of object it is
This will be done
• Anchored Note, page 93
Make "template" private
This will be done
• Anchored Note, page 95
Again, rename this!
Rob and Karla would like to come up with a new name.
This is the same ETC issue above
• Anchored Note, page 95
Multiple Exit Points
Tom's concerned about multiple exit points
We noted the concern but see it as a style difference.
• Anchored Note, page 98
Fix the hack
We copied a "HACK" from the prototype - need to clean it up
A new PR was filed for this. (PR 61458)
• Anchored Note, page 98
Should the calculator be stateless?
Andy said we discussed making this stateless at some point. Revisit.
It was made into a static, stateless calculator that returns the needed
values.
• Anchored Note, page 98
Style Discussion
Tom wants a style discussion on how to write these constraint methods - should the calculate method just return the value of another method?
This was resolved in comments above, regarding how we are identifying Constraints automatically
with annotations.