Skip to content

Firelfy-838: Extraction of data from a FITS image#1136

Merged
robyww merged 2 commits intodevfrom
firefly-838-cube-extractor
Oct 11, 2021
Merged

Firelfy-838: Extraction of data from a FITS image#1136
robyww merged 2 commits intodevfrom
firefly-838-cube-extractor

Conversation

@robyww
Copy link
Copy Markdown
Contributor

@robyww robyww commented Oct 6, 2021

Firelfy-838: Extraction of data from a FITS image

  • Cube z-axis extraction
  • Line extraction across an imagae
  • Points extraction by click on points
  • Direct extraction from fits file, no extra data read into memory
  • UI syncing the images, charts and tables together after extraction
  • New icons for all three extractions types
  • All images are now uncompressed
  • Create a spectral data model for cube extraction with wavelength data
  • If there is a HDU name that begin with ERR use that for spectrum error
  • Improved Catalog drawing layer to handle catalogs of image pt

ticket: https://jira.ipac.caltech.edu/browse/FIREFLY-838

Testing

  • url: https://fireflydev.ipac.caltech.edu/firefly-838-cube-extractor/firefly/
  • reading in a fits cube or plain FITS image.
  • Us tool pull down to find 3 types of extraction.
  • Line and point work on any fits
  • Z-axis works on cubes
  • Results immediately
  • Use Keep to create a table
  • Use save to save the table right away.
  • Spectral cubes will create a spectral chart when you keep

  - Cube z-axis extraction
  - Line extraction across an imagae
  - Points extraction by click on points
  - Direct extraction from fits file, no extra data read into memory
  - UI syncing the images, charts and tables together after extraction
  - New icons for all three extractions types
  - All images are now uncompressed
  - Create a spectral data model for cube extraction with wavelength data
  - If there is a HDU name that begin with ERR use that for spectrum error
  - Improved Catalog drawing layer to handle catalogs of image pt
@robyww robyww added enhancement Image FITS images Charts Anything related to charts labels Oct 6, 2021
@robyww robyww self-assigned this Oct 6, 2021
@lrebull
Copy link
Copy Markdown
Contributor

lrebull commented Oct 6, 2021

Do you have sample FITS files for us to try?

@robyww
Copy link
Copy Markdown
Contributor Author

robyww commented Oct 6, 2021

Sent

@ejoliet
Copy link
Copy Markdown
Contributor

ejoliet commented Oct 7, 2021

@lrebull
Copy link
Copy Markdown
Contributor

lrebull commented Oct 7, 2021

Wow, this is really cool.
trying the "drill" icon tool.

  1. I know you probably don't want detailed comments like this, but a possible ambiguity in wording -- "download", empirically, refers to downloading the data table, but because this is a plot, i thought initially that it meant to download the plot. Maybe that can be a choice in the pop-up? or one of the choices in the download file format?
  2. After I click "keep", and the pop-up window is closed, the main image pane jumps back to the first plane, as opposed to the plane I left it on (15).
  3. If you try to make it go to a plane by typing in a number larger than the number of planes, it just drops you back in the first plane. (obviously not a deal breaker, but had me confused for a bit.)
  4. After messing around with it for a while, i have two target markers on my image, and i can't make them go away. Were they supposed to be the locations of the drill down? I've now deleted all the tables and they are still there. I tried again, and now those target markers sure seem like they are supposed to be the locations of the drilldown, but they are offset. (see movie attached next item)
  5. when scrolling through the rows in my "kept" drilldown, the image jumps, or rather the markers jump? maybe both? see movie: https://user-images.githubusercontent.com/23038600/136466471-468b5fd8-9422-419a-99bf-898a77c27701.mp4

I haven't tried anything other than the "drill" icon tool yet. More later.

@robyww
Copy link
Copy Markdown
Contributor Author

robyww commented Oct 7, 2021

  1. I know you probably don't want detailed comments like this, but a possible ambiguity in wording -- "download", empirically, refers to downloading the data table, but because this is a plot, i thought initially that it meant to download the plot. Maybe that can be a choice in the pop-up? or one of the choices in the download file format?
  • Good Suggestion: I will make it Download Table and add a Download Chart.
  1. After I click "keep", and the pop-up window is closed, the main image pane jumps back to the first plane, as opposed to the plane I left it on (15).
  • FIXED: I was aware of that but your comment helped me think of a solution.
  1. If you try to make it go to a plane by typing in a number larger than the number of planes, it just drops you back in the first plane. (obviously not a deal breaker, but had me confused for a bit.)

I would have to see this one, I though it gave you the error indicator. It does for me, maybe you are doing something different.

UPDATE: After watching @lrebull 's movie... It is an existing behavior, so it is of scope for this PR.

  1. After messing around with it for a while, i have two target markers on my image, and i can't make them go away.
  • They go away for me. I will study your movie.
  1. when scrolling through the rows in my "kept" drilldown, the image jumps, or rather the markers jump?
  • FIXED: I am going to center the marker points.

Details:

The markings are the point that was extracted. They are on the bottom left of the pixel. I will make them center on the pixel. You will see this again at other times like line extraction. This is pretty standard for fits viewers. If I plot the point 5,5 on a image with a zoom level of like 20 you will see them on the bottom left corner. To center them I have to make the point 5.5,5.5. That is fine for the markers. However, line extraction that shows up in the table will plot them on the bottom left, because it is plotting the table data.

The jumping is the image repositioning itself. Sometime it is slow enough to see. I think it ends up in the right place. So, for now at least, I am not going to do anything about it.

@lrebull
Copy link
Copy Markdown
Contributor

lrebull commented Oct 7, 2021

video demonstrating my #3 which you said you couldn't reproduce
https://user-images.githubusercontent.com/23038600/136476082-e5cc515e-fee9-4cdd-bf72-2e139322644c.mp4

@lrebull
Copy link
Copy Markdown
Contributor

lrebull commented Oct 7, 2021

checking new version from slack. markers still seem to me to be offset but maybe it's just the big pixels ?

Ooh, a help anchor has appeared! I need to spend some time figuring out what the docs framework needs to be - i may very have to reorganize substantially to accomodate the rework (not just the new toolbar stuff but the reorg may mean the docs need to be comprehensively changed). I'm not sure I'll really be able to "see" the new org until i see more of the new tool, but i can try.

@robyww
Copy link
Copy Markdown
Contributor Author

robyww commented Oct 8, 2021

@lrebull thanks for the video. This is out of scope for this ticket. It has been the existing behavior for over a year, maybe two. Right now I am not inclined make a ticket to change it. If you feel very strongly that the behavior is wrong I suppose you can make a ticket. It just seems very low priority for something that is not really a bug.

Copy link
Copy Markdown
Contributor

@loitly loitly left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great PR. Lots of features added. I do like you to reconsider using the existing
defaultChartDef(MetaConst.DEFAULT_CHART_DEF) feature instead of creating the new method. It's allow you to do what you want to do, plus more without much complexity.

List<FitsReadUtil.ExtractionResults> results,
String wavelengthColName, String fluxColName) {

dataGroup.getTableMeta().addKeyword("utype", "spec:Spectrum");
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use constant TableMeta.UTYPE instead.

TableMeta meta= dataGroup.getTableMeta();
meta.addKeyword(MetaConst.DEFAULT_CHART_X_COL, xColName);
meta.addKeyword(MetaConst.DEFAULT_CHART_Y_COL, defYCol);
dataGroup.trimToSize();
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think trimToSize is needed. You did not touch the data.

Comment thread src/firefly/js/charts/ChartUtil.js Outdated
isTableLoaded,
stripColumnNameQuotes,
watchTableChanges
} from '../tables/TableUtil.js';
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You mentioned in the past that you would revisit this auto style's option.

Comment on lines +1000 to +997
// test to see if meta set the default x and y coloumns
const additionalChartDef= getMetaEntry(tblModel,MetaConst.ADDITIONAL_CHART_DEF);
let xCol= getMetaEntry(tblModel,MetaConst.DEFAULT_CHART_X_COL);
let yCol= getMetaEntry(tblModel,MetaConst.DEFAULT_CHART_Y_COL);
if (xCol) {
return genericXYChart({tbl_id, xCol, yCol:yCol||xCol, additionalChartDef});
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure this is the best approach. It's very specific to scatter plot and yet genericXYChart can create a Histogram. It also relies on additionalChartDef which is also JSON based.

Have you looked into using defaultChartDef(MetaConst.DEFAULT_CHART_DEF) instead?
It can be as simple as this javacript equivalent, yet allow you to make it as complex as you like.
META_INFO: {defaultChartDef: JSON.stringify({data: [{x: 'tables::w1mpro', y: 'tables::w2mpro', mode: 'markers'}]})}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have thought about this this a lot. I think we need to specify the columns. Now maybe we could use one instead of two such as wmpro1;wmpro2 using DEFAULT_CHART_DEF is very complex to every use case. The ADDITIONAL_CHART_DEF was an experiment that I am not using, I meant to take out. I will do that. But we need a way to just specify the columns.

…esponded to feedback

  - added a download chart
  - made cube not reposition when extraction table is loaded
  - make z-axis markers center on image point
  - removed additionalChartsDef
@robyww robyww force-pushed the firefly-838-cube-extractor branch from 1304c8b to 870b3f5 Compare October 11, 2021 19:50
@robyww robyww merged commit e720d79 into dev Oct 11, 2021
@robyww robyww deleted the firefly-838-cube-extractor branch October 11, 2021 19:58
@robyww robyww added this to the 2021.4 milestone Oct 12, 2021
@ejoliet
Copy link
Copy Markdown
Contributor

ejoliet commented Oct 13, 2021

I know it was merged, but i'd like to give my notes here for the records i couldn't comment before as i was on vacation.

Layer settings?

  • Any better way to give the information ? Extraction Point
    (6, 20) / space coordinate instead if exists?
  • “Selected point” - maybe a better wording that refer to the drill feature? “Drilled point”?

Pop up dialog

  • Improvement: why 5x5 and not custom N x M?
  • when selected 5x5 pixels, it resets after closing the drill feature, does it matter? Should we keep this next time the user open it?
  • “send to time series”?

Note: Found very useful that the dot in the chart is connected to the plane index.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Charts Anything related to charts enhancement Image FITS images

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants