Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BUG] Undo not working after addition of annotation building block. #72

Closed
Brilator opened this issue Dec 2, 2020 · 9 comments
Closed
Labels
Host: Excel Issues regarding Excel interop Type: Bug Something is not working, and it is confirmed by maintainers to be a bug.

Comments

@Brilator
Copy link
Member

Brilator commented Dec 2, 2020

Describe the bug
Excel "Undo" is not working properly after addition of an annotation block.

To Reproduce
Steps to reproduce the behavior:

  1. Go to 'Annotation building block selection'
  2. Search for any term to add as annotation column.
  3. Click on 'Annotation building block'
  4. Try to undo (ctrl+z or edit-> undo)
  5. See interesting behavior, which I cannot properly put into words. It at least does not simply delete the annotation block, which is probably due to multiple columns being added in parallel (including the hidden ones).

Expected behavior
Undo should undo the complete last action of annotation block addition.

OS and framework information (please complete the following information):

  • OS: macOS Catalina
  • OS Version 10.15.7
  • MS Excel: Excel Desktop
  • MS Excel Version 16.43

Additional context
I must admit that I also find the hidden cols a bit annoying. I understand the rationale to not overwhelm standard users with ontologies. But I'd rather try to teach users why ontologies matter than hiding data in the background.

@Brilator Brilator added the Type: Bug Something is not working, and it is confirmed by maintainers to be a bug. label Dec 2, 2020
@Brilator
Copy link
Member Author

Brilator commented Dec 2, 2020

Ah, sorry. I think this is kind of related to #48.

@Freymaurer
Copy link
Collaborator

This is actually related to the issue mentioned here: #9 .
We are currently not able to track changes done by the add-in and i think it even deletes all changes it tracked to that point. I am not sure if we even can fix that issue without implementing a version history on our own.

@Brilator Your feedback is very welcome! Thank you for all your input.

@kMutagene
Copy link
Member

kMutagene commented Dec 2, 2020

Regarding undo functionality, this is disabled by-design of the underlying office.js library (and at that, specific to excel). Not much we can do about that.

I agree that there could/should be an option to disable the auto-hiding though.

The problem i see with the general format is that the ontology references do not add much to the flow of reading such an annotation file (from the 'what happened from source to sink' perspective). Ontologies matter, but i am not sure if a column with 20 times the same url does really help at that. I think making a lot more settings optional/togglable is the way to go here, and maybe enable saving settings down the line so you don't have to unhide those columns every time if you are interested in them. At the end we want to enable both styles of working here, more in-depth focus on the annotations as well as straight-forward workflow annotation

@kMutagene kMutagene added the Host: Excel Issues regarding Excel interop label Dec 2, 2020
@Brilator
Copy link
Member Author

Brilator commented Dec 2, 2020

Hey, no worries, there will be more input...
You're doing a great job with ARC, SWATE and annotation principles!
Not sure if Timo explained, I'm Dominik from Düsseldorf (HHU, CEPLAS). GitHub Newbie btw, so don't hesitate to let me know if I have to do things differently (e.g. issue templates are helpful).

@Brilator
Copy link
Member Author

Brilator commented Dec 2, 2020

One more thing here, since I'm not sure wether this is a bug or added feature:
Some characteristics that may be specific to a lab or facility (e.g. metabolomics) will probably never have a "Term Source REF" and "Term Accession Number" (for example the "LabelText" in your anno principles slide). Is it necessary to still automatically add these to stay ISA compliant?

@kMutagene
Copy link
Member

kMutagene commented Dec 3, 2020

@Brilator Timo mentioned that there will be feedback coming in from you. Don't worry, your input is very valuable.

Some characteristics that may be specific to a lab or facility (e.g. metabolomics) will probably never have a "Term Source REF" and "Term Accession Number" (for example the "LabelText" in your anno principles slide). Is it necessary to still automatically add these to stay ISA compliant?

There are two answers for that. First, you can annotate a characteristic with free text, without using an ontology term. See here. The accession number and term source ref fields are only required when there is an ontology term in use. When such a term does not exist, these columns can stay empty.

The preferred approach to this would however be filing a missing term issue at our ontology so we can add the missing term to our database (which will also lead to the terms being available in the term search). That way, we can ensure to both fill the ontology gap and make sure that workflows are as well annotated as possible.

@Brilator
Copy link
Member Author

Brilator commented Dec 3, 2020

Yes sure, I know you can add free text and leave cells blank for ontology-unrelated terms. But this answered my question - it's required for ISA compliance.
And sure in the long-term we'll want to have everything referenced via ontology / PIDs, but there will always be user/lab-specific processing steps (e.g. storage location of samples / extracts: the 20 °C freezer in basement room 4 - board 6 - box 2). These will not matter for downstream analyses, but I would still want to include for complete traceability and to push user-friendliness (i.e. everything in one place).

@kMutagene
Copy link
Member

Sorry i misunderstood the question. When there is no intent to add a term later for such a characteristic, the empty columns are not required, as ontology annotations themselves are optional in the ISA format.

@Brilator
Copy link
Member Author

Brilator commented Dec 3, 2020

Ah, alright. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Host: Excel Issues regarding Excel interop Type: Bug Something is not working, and it is confirmed by maintainers to be a bug.
Projects
None yet
Development

No branches or pull requests

4 participants