Quasars / orange-spectroscopy

Other
51 stars 59 forks source link

[FIX] Agilent Tile Reader: Generate correct y-axis positions #632

Closed stuart-cls closed 2 years ago

stuart-cls commented 2 years ago

I found that the agilentMosaicTileReader had an error in the way it calculated the y-axis positions. Although the agilent data xy positions are arbitrary from (0,0), it means the TileReader and the normal reader generate different positions.

I'm asking for input because this has implications for backwards compatibility. In the case where someone uses the OWTileFile widget and then selects regions in OWHyper, the selection will be incorrect when that workflow is loaded with this code.

Thoughts?

Edit: This now fixes both the coordinates and the table row ordering so that the two readers result in identical output.

codecov-commenter commented 2 years ago

Codecov Report

Merging #632 (42358c2) into master (8dc9d14) will increase coverage by 0.02%. The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #632      +/-   ##
==========================================
+ Coverage   89.25%   89.28%   +0.02%     
==========================================
  Files          71       71              
  Lines       11767    11767              
==========================================
+ Hits        10503    10506       +3     
+ Misses       1264     1261       -3     
markotoplak commented 2 years ago

So, is it still a problem after your edit? :)

Hmm, it is always hard to decide whether to keep backward compatibility in cases like this. I try to do it if it is reasonably possible (with that "if old settings are different behave differently trick"). But sometimes, if we judge the use-case to be particularly rare, I am fine with forgoing compatibility. Your call.

stuart-cls commented 2 years ago

I double checked, and yes: ROI-old-tilereader ROI-new-tilereader

I think in this case it would be quite difficult to special case, I suppose it could be done in the OWTile with a special deprecated Reader?

I will confer with some users of this and then decide.

markotoplak commented 2 years ago

Yes, as you say, you'd need different reading code...

stuart-cls commented 2 years ago

Decided to skip backwards compatibility which would add significant complexity. Impact is only on single-dataset archival workflows, which can be opened using old versions if necessary.

markotoplak commented 2 years ago

Thanks!