FIREFLY-2066: Consolidate the loading UI of Spectral Lines feature - #2015
jaladh-singhal wants to merge 21 commits into
Conversation
…nd plotting clear
Fix the layout styling issues with NoDataTableView
| // Recommended spectral line lists in the Spectral Lines panel: a JSON array of {label, src} objects, in order. | ||
| // Omit "src" to use one of Firefly's bundled lists - any other label with no src is dropped (logged as an error). | ||
| // Set to "[]" to offer no spectral line lists at startup. | ||
| charts.spectrum.linelists = "[{\"label\": \"SPHEREx line list\"}, {\"label\": \"Spitzer PAHFIT line list\"}, {\"label\": \"Herschel HSPOT line list\"}, {\"label\": \"JWST line list (remote)\", \"src\": \"https://gist.githubusercontent.com/jaladh-singhal/2b4230e2fc64586fbe7b51519d26ad3f/raw/21f503d13bc0e859d269d8acc782083f7fa84c7e/jwst_linelist.tbl\"}]" |
There was a problem hiding this comment.
NOTE: Last option "JWST line list (remote)" is just a test for recommended/pre-installed line list coming from a remote server rather than bundling with firefly static resources.
I plan to remove it when merging this PR.
|
This is so cool! See here for two items, at least on my screen: 
Item 1: The labels are a tiny bit too far away from their line. I've weeded down this list, but even so, on the edges you can see that the label for "OCS" is actually closer to the line for "13 CO" than it is its own line, and same for Paschen gamma -- it's closer to Paschen delta than its own line. These need to be a bit closer to the corresponding line, IMHO. Item 2: you can see here how a lot of the labels run off the top. Less than ideal. Is there a way for users to tweak this? move them down? I love the dynamic response of the pop-up. At the end of the process, we have this at the bottom of the pop-up: 
We have trained people elsewhere that clicking on the green version of that little plot icon does things. Now it's blue, and clicking on it doesn't do anything. Is that good UI/UX? The arrow at the end also doesn't do anything. What is it meant to indicate? I mean, the first thing I did with the pop-up was grab and move it to cover the image in the tri-view so that i could see the plot, and maybe that makes me different than you expected? Ooh, this should not be default behavior: even if the line list goes out past 50 um, if the data only go to 8.6 um, the plot, even after adding the line list, should only go to 8.6 um. The priority should be to show me the data, not the line list. Actually, this is a problem. Look, I've tried to weed the line list down to only be <8.6 um, and the table is filtered appropriately (see scrollbar on the line list), but the plot is showing me lines >8 um. In any case, I think what we need to do is echo the redshift to the top of the spectral line list. In between the "select line lists to load" pulldown thing, and before "select lines to plot", I propose something in italics: "redshift has been applied to lines in plot; see plot options" or something better than that. (i'm sure others will Have Opinions.) The idea is to get a pointer or hint or something in here, because I, for one, didn't understand what could possibly be going on here. Let me see if I can get some other scientists in on this. |
|
@lrebull thanks for the feedback.
I'm not seeing this issue in Chrome - the labels are sticking to the lines. But I can see this issue in Firefox assuming that's what you used for testing? Will debug why and try to fix.
Yes, I meant to fix this but forgot. And now it's more pronounced with looong labels in the lists you all gave me. Will fix.
This is just a decorator to indicate that "hey, look beneath this popup where results screen is". I can see the click-behavior confusion due to blue color. How about I revert it to the same color of text (grey-ish)? OR do you want me to remove it altogether? I'm also open to changing the wording of response text to incorporate some version of "look beneath in results" if you have any better suggestions.
Hmm, I think this happens because this what plotly is doing by default to show all elements you plotted on chart. I'll have to override the chart layout to always only show the range of data traces. Will try to fix.
I changed the original redshift value in the file (from 1e-3 something) to 0.5 so that redshift correction is easily noticeable. Please disregard the scientific accuracy for just this spectrum file that has hand-crafted metadata 😄
This becomes clear if you open the chart options dialog because it tells that redshift correction is being applied to spectral lines in observed frame whereas to spectrum in rest frame. Because we settled on this one spectral line dialog to rule them all, I cannot show any wording specific to one spectral chart since it may be irrelevant for other spectral charts open. How about right after the table we add a note: "If spectral chart is in observed frame, redshift gets applied to the selected lines in the plot; see Chart Options dialog"? I plan to keep it after the table because the lines in table are always in rest frame, it's only when plotting (after table selection before "n lines plotted" response) it gets determined where they land on wavelength axis. |
robyww
left a comment
robyww
left a comment
There was a problem hiding this comment.
Good job. I found it go be working as planned and I don't see any bugs.
Code
A couple of small suggestions and I want to have a conversation about how we generate ids and how the person doing firefly config uses them. I don't think we have it right yet.
UI
- The is a UI issue with the Upload table tables dialog. The dialog is coming up too narrow and the
Table Metasection is not fully displayed.
- in
Spectral Lines Optionthe table sometime looks like this:
| }; | ||
| }, [chartId]); | ||
|
|
||
| useSpectralLinesSync(chartId); |
There was a problem hiding this comment.
is it necessary to the a custom hook if this is the only place it is use? Or is there a plan to use it elsewhere?
| const {xErrArray, yErrArray, xMax, xMin, yMax, yMin, xUnit, yUnit} = getSpectrumProps(tbl_id); | ||
|
|
||
| const {Xunit, Yunit, SpectralFrame, SpectralLines} = useSpectrumInputs({activeTrace, tbl_id, chartId, groupKey}); | ||
| const {Xunit, Yunit, SpectralFrame} = useSpectrumInputs({activeTrace, tbl_id, chartId, groupKey}); |
There was a problem hiding this comment.
useSpectrumInputs is written as a hook but it looks like it is only a utility function. Maybe it should be useSpectrumInputs
| "SPHEREx line list", "/edu/caltech/ipac/firefly/resources/spherex_lines.tbl", | ||
| "Spitzer PAHFIT line list", "/edu/caltech/ipac/firefly/resources/pahfit_lines.csv", | ||
| "Herschel HSPOT line list", "/edu/caltech/ipac/firefly/resources/hspot_lines.csv" |
There was a problem hiding this comment.
After looking at this. I have two concerns.
- because there is nota separate id only an id build from the title, If the title get changed (at it will) then another app the is specifying just one will break.
- This is really only only place our predefined lines list is documented. I don't have a solution it is just a concern.
I think you, I and @loitly should have a discussion about this before you merge.
I think the ID should be something like.
SPHEREx-v1Spitzer-PAHFITHSPOT
There was a problem hiding this comment.
Yes, let's talk about it. I don't quite understand the concerns.
| try { | ||
| File tempFile = createTempFile(req, isUrl ? null : info.src().substring(info.src().lastIndexOf('.'))); | ||
| if (isUrl) { | ||
| URLDownload.getDataToFile(new URI(info.src()).toURL(), tempFile); |
There was a problem hiding this comment.
check the status in the FileInfo return
| info.listLabel(), info.src(), WAVELENGTH_COL)); | ||
| } | ||
| return dg; | ||
| } catch (Exception e) { |
There was a problem hiding this comment.
getDataToFile throws a FailedRequestException in certain very bad cases. You should check for that here as well.
if (e instanceof FailedRequestException fre) {
int code= fre.getResponseCode();
// log something with code
}
|
Yes, i'm using firefox. Yeah, i really don't want to use the 'results' icon in a way differently here from where it gets used elsewhere; that seems messy. let me see if can rally the other scientists for input on this one ... A note after the table would work. again, need input from other scientists ... |
loitly
left a comment
loitly
left a comment
There was a problem hiding this comment.
UI worked great. Just a few suggested changes.
| private static List<LineListInfo> parseLineListsConfig() { | ||
| List<LineListInfo> lists = new ArrayList<>(); | ||
| try { | ||
| JSONArray entries = (JSONArray) new JSONParser().parse(AppProperties.getProperty("charts.spectrum.linelists", "[]")); |
There was a problem hiding this comment.
I would define AppProperties.getProperty("charts.spectrum.linelists", "[]") as a constant.
Also, I would like to slowly move away from simple JSON in favor of Jackson API.
edu.caltech.ipac.util.serialization.Serializer#fromJson(byte[], java.lang.Class)
Try replacing your line with:
var entries = Serializer.fromJson(CHART_SPECTRUM_LINELISTS, Map<String,String>[].class);It will do all of the type casting, making easier to read.
| String src = (String) entry.get("src"); | ||
| if (src == null) src = BUNDLED_RESOURCES.get(label); | ||
| if (src == null) { | ||
| LOGGER.error("charts.spectrum.linelists: no bundled resource for label \"" + label + "\" - dropping from spectral lines list"); |
There was a problem hiding this comment.
Maybe charts.spectrum.linelists should be the constant so you don't duplicate it. Then, use formatted to supply the values.
"%s: no bundled resource for label \"%s\" - dropping from spectral lines list".formatted(CHART_SPECTRUM_LINELISTS, label);
| DataGroup dg = TableUtil.readAnyFormat(tempFile, 0, req); | ||
| DataType wlCol = dg.getDataDefintion(WAVELENGTH_COL); | ||
| if (wlCol == null) { | ||
| LOGGER.warn(String.format("Spectral line list \"%s\" from %s: \"%s\" column is missing - no lines will be loaded from this list.", |
There was a problem hiding this comment.
String.format("
FYI, "abc".formatted() was added later to make it easier to read.
| JSONArray lists = new JSONArray(); | ||
| LINE_LISTS.forEach(info -> { | ||
| JSONObject o = new JSONObject(); | ||
| o.put("listId", info.listId()); | ||
| o.put("listLabel", info.listLabel()); | ||
| lists.add(o); | ||
| }); |
There was a problem hiding this comment.
edu.caltech.ipac.util.serialization.Serializer#toJsonString
Jackson API version.
|
|
||
| python.exe= @python.exe@ | ||
|
|
||
| charts.spectrum.linelists=@charts.spectrum.linelists@ |
There was a problem hiding this comment.
If you want this property to be appear in apps built using Firefly, place it in common.prop instead.
4 participants
Fixes FIREFLY-2066
SpectralLinesPanel- loading is a separate concern from per-trace display options, and the panel is shared app-wide, not per-chartcharts.spectrum.linelistsapp-config property ({label, src?}array,srccan be a bundled resource or remote URL) instead of one hardcoded combined CSV - retiredlinelist_combined.csvfor separatehspot_lines.csv/pahfit_lines.csv/spherex_lines.tblresources plus a remote JWST list as a src-as-URL examplespectral-lines), rebuilt only on explicit "Load Lines"/"Clear All", never automatically on checkbox/mapping changeSpectralLinesProcessorwarns server-side when an available line list's wavelength column has no units metadata, and the upload column mapper shows similar feedback for the uploaded file's wavelength columnAdditional changes/cleanup:
UploadTableSelectorenhanced for use inSpectralLinesPanel: addedallowClear(adds a chip to clear the uploaded file),uploadTblOptions(keeps the upload out of the main Results view), and a per-fieldgetFeedbackhook (surfaces the uploaded wavelength column's detected/assumed unit)SpectrumOptions: Standardized default resolution logic for redshift options (getEffectiveSpectralFrameOption) and de-duplicated a few hardcoded spectral-frame/redshift constantsSpectrumUnitConversion(for SPHEREx spectrum results)BasicTableViewTesting
All the test files linked in the ticket.
https://firefly-2066-spec-lines-loading.irsakubedev.ipac.caltech.edu/firefly
Core functionality
Upload line list
Redshift correction
0.5and "Apply" -> spectral lines should redshift right of their lines-table wavelength. Switch to "Rest Frame" with the same redshift and "Apply" -> lines return to their original wavelength and the spectrum blueshifts left instead. Net effect: lines and spectrum end up in the same relative position, just with the X-axis shifted - confirming the correction applies correctly in both framesRegression