m2ms/fragalysis-frontend

Fix tag generation bugs in loader

Opened this issue · 19 comments

From #1322 it's clear some tags are being generated with the wrong strings.

Image

@phraenquex to full spec and update @Waztom before he goes offline.

@kaliif - @Waztom will discuss this spec with you.

  • In metadata.csv, split short and long codes into separate fields (contrary to Frank's original spec)

  • conformer_site should be the full string, e.g. A71EV2A-x0526+A+147+1 (currently it's truncated before the first +)

  • canonSite

    • should be the root string, e.g. A71EV2A-x0526+A+147+1, i.e. first half of longCode field (currently it's populated by centroid_res)
    • it needs an alias field in the B/E, that users can edit, but is initially populated by the canonSite string
    • both canonSite string and alias should be written into metadata.csv (currently it has only the short code and alias, with the string hiding in the longCode field)
    • both canonSite string and alias should be displayed in the extended observation panel
  • centroid_res

    • should be loaded (new DB field?), and written into metadata.csv
    • show centroid_res in the expanded observations pane, at the very right (small action for @matej-vavrek)
  • CrystalformSite should be the full root string, e.g. A71EV2A-x0451/A/201/1 (currently it truncates the slash)

  • Need an "ObservationID", (the first half of the long code) - or whatever it's called in the yaml files, if it's there (we couldn't see it)>

    • written into metadata.csv
    • displayed as left-most entry in the expanded observations panel

Updated the tags and metadata.csv. centroid_res was previously missing from the db, which means new field was added, which means database migration. It's not a breaking change, the old data keeps working but for the field to be populated the data needs to be reloaded. @matej-vavrek once the changes go live, the field is available in api/canon_sites/. Let me know if there's any information you need that's currently missing from the API.

ObservationID is now present in metadata.csv, labeled as Experiment code, that's how it's saved in the db. Can change the name if necessary.

Somewhat confused about the first point though --

  • In metadata.csv, split short and long codes into separate fields (contrary to Frank's original spec)

@phraenquex isn't this already happening? The first two fields, Code and Long code?

@kaliif short and long code are in a single field, joined by a dash, in the metadata.csv that is downloaded from staging. But you can quickly check yourself.

@Waztom please check with @ConorFWild whether centroid_res is not after all the better string to use for the canonical site long code. (As we discussed by phone, but didn't document in this ticket.)

@kaliif short and long code are in a single field, joined by a dash, in the metadata.csv that is downloaded from staging. But you can quickly check yourself.

Checked and as far as I can see they are clearly separate fields - short code is the first column, long code second.

Currently, it is showing upload_name without part before first - for each tag. If some category should use something else to show, specify it please, because I am confused a bit now 🙄

@matej-vavrek the two fields to add to the Observation Modal are: the TagName (left-most-side) and the CentroidRes (right-most-side).

@kaliif will confirm endpoints with you.

Image

@kaliif has these changes on his stack, but there is currently no LHS dataset. @mwinokan will upload a 2A dataset and have a look. Check against spec

Testing (2024-08-06), omitting f/e stuff:

Checked means it's implemented

  • In metadata.csv, split short and long codes into separate fields (contrary to Frank's original spec)
  • conformer_site should be the full string, e.g. A71EV2A-x0526+A+147+1 (currently it's truncated before the first +)
  • canonSite should be the root string, e.g. A71EV2A-x0526+A+147+1, i.e. first half of longCode field (currently it's populated by centroid_res). *This has not been fixed as of this round of testing
  • canonSite needs an alias field in the B/E, that users can edit, but is initially populated by the canonSite string. This may be working after a CanonSite tag is renamed in the f/e but I don't have permissions so can't check.
  • both canonSite string and alias should be written into metadata.csv (currently it has only the short code and alias, with the string hiding in the longCode field)
  • centroid_res should be loaded (new DB field?), and written into metadata.csv
  • CrystalformSite should be the full root string, e.g. A71EV2A-x0451/A/201/1 (currently it truncates the slash)
  • Need an "ObservationID", (the first half of the long code) - or whatever it's called in the yaml files, written into metadata.csv. This is duplicated by showing the original CanonSite name so not needed.

@kaliif please revisit the outstanding three bullets of the spec, above

@Waztom clarifies that the aliases referred to in @phraenquex's spec and @mwinokan's testing refer to the string tag's given to the SiteObservations, so there is no need for new B/E objects.

However, the metadata download should contain the XCA generated string and the user-defined tag string for all the XCA generated site/tag types.

@kaliif will please revisit if this has been implemented and update his stack

@matej-vavrek will revisit the styling once the backend has been updated

Your latest stack looks good to me, thanks @kaliif

@mwinokan, time to merge to staging or shall we wait for the decision on splitting the long and short codes?

@kaliif I say merge it so that @matej-vavrek can work on the f/e

@mwinokan, @Waztom, I have updated my stack. There are also TagName and CentroidRes columns now.

@matej-vavrek thank you, one outstanding issue is that the headings are not aligned to the data values. If this is a simple fix please implement that.
Screenshot 2024-08-20 at 12 05 28

@mwinokan, I have adjusted it a bit, but this is probably the farest I can go with a simple fix. It is on tibor stack (mine is down atm).

@matej-vavrek it looks much better, thank you

@matej-vavrek this is ready for staging