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

Key-value pair scripts enhancement #216

Open
wants to merge 130 commits into
base: develop
Choose a base branch
from

Conversation

Tom-TBT
Copy link

@Tom-TBT Tom-TBT commented Feb 2, 2024

Hello,
here are the four scripts around key-value pairs, enhancing three and adding a new one to convert key-value pairs.

Import_from_csv.py: Import key-value pairs and tags from a CSV file
Export_to_csv.py: Export key-value pairs and tags to a CSV file
Remove_KeyVal.py: Delete key-value pairs
Convert_KeyVal_namespace.py: Changes the namespace of key-value pairs

Feel free to propose different names for the scripts, parameters, or UI changes.

Current repo for the associated guide:
https://github.com/German-BioImaging/guide-KVpairs-scripts/

@Tom-TBT
Copy link
Author

Tom-TBT commented May 8, 2024

@will-moore please approve the workflow. I adjusted the script to your suggestions. Thanks for the review

@Tom-TBT
Copy link
Author

Tom-TBT commented Jun 5, 2024

Hello @will-moore @jburel,
is there anything left blocking the merge of this PR?

Cheers

ping @pwalczysko

@pwalczysko
Copy link
Member

I was thinking we could use this on our next outreach cc @jburel

@jburel
Copy link
Member

jburel commented Jun 5, 2024

@Tom-TBT you should add yours and @JensWendt institution e.g. Gerbi when indicating your name

Copy link
Member

@will-moore will-moore left a comment

Choose a reason for hiding this comment

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

All good from me, thanks!

@Tom-TBT
Copy link
Author

Tom-TBT commented Jun 5, 2024

I added our institute affiliations

@Tom-TBT
Copy link
Author

Tom-TBT commented Jun 11, 2024

Anything else from you @jburel? Sorry for being pushy, I'm trying to close that topic.

@joshmoore
Copy link
Member

One heads up in terms of versioning: the removal or renaming of scripts in my mind would count as a breaking change.

@will-moore
Copy link
Member

So it would mean a v5.9.0 tag once this is merged?

@sbesson
Copy link
Member

sbesson commented Jun 11, 2024

One heads up in terms of versioning: the removal or renaming of scripts in my mind would count as a breaking change.

Seconding that. In my mind that would almost justify calling the next release of omero-scripts as v6.0.0.

Semi-related, is there any reference documentation about the existing scripts that would need to be updated? And for end-users who would have been using the previous key/value scripts in production, are there simple upgrade recommendations?

@Tom-TBT
Copy link
Author

Tom-TBT commented Jun 12, 2024

About documentation, I have made documentation for the scripts, that lives on its own for now. The plan was to merge it with your doc after the merge of this branch is completed, if I recall correctly what JM said.

The repo:
https://github.com/German-BioImaging/guide-KVpairs-scripts

The doc:
https://guide-kvpairs-scripts.readthedocs.io/en/latest/index.html

@Rdornier
Copy link

Hello,

Apologize for entering the game so late. I've tested the scripts and I found them very good !

I have a suggestion regarding the csv file that is generated. Would it be possible to add the separator used when it is generated (by adding in the first line sep=%separator% ) ?

This will ease the csv file opening and saving with Excel. For example, I have an Excel configured to recognize commas as separator. If the image names don't contain any comma, that's fine but if they contain some, it becomes a mess. I already give those scripts to some users and I made them aware of such issues but it is always better to force reading the file with the correct separator.

I hope it's not too late for that.

Rémy.

@Tom-TBT
Copy link
Author

Tom-TBT commented Jul 15, 2024

Hi @Rdornier,
I was not aware that such magic was possible for Excel. I simply thought it was poorly written because with Open Office (on my home computer) I can open CSV files without this problem.

The annoying trick I used with Excel is to open a blank sheet and in Data -> From Text/CSV select your CSV file. Only then Excel is capable of detecting the separator. So I agree to have it easier.

Two issues I can fix:

  • writing the CSV better (quotes around the text so commas are fine with the export of comma as separator)
  • using that metadata in the first line. Should be minor changes

Thanks a lot for the feedback. That shouldn't be too late

@Rdornier
Copy link

I was not aware that such magic was possible for Excel

I didn't know neither until today... I found this on stackoverflow

@Rdornier
Copy link

Hi @Tom-TBT

I have another feedback for you. When I try to convert a namespace from default_namspace to A_namespace, with the checkbox to create new and merge activated, it converts the KVP namespace correctly but doesn't merge the two KVPs together (before running the script, some KVPs were already existing under A_namespace).

To be able to merge the two KVPs in one, I had to run again the script, convert from A_namespace to A_namespace and check the box.
I think it is better if we can remove the extra run.

Thanks,
Rémy.

@Tom-TBT
Copy link
Author

Tom-TBT commented Jul 16, 2024

Oh, I see. The extra run could currently be avoided by giving to "Old Namespace" the two namespaces as input:
openmicroscopy.org/omero/client/mapAnnotation, A_namespace

But ticking "create new and merge" seems like an explicit agreement to merge all key-value pairs into a single A_namespace. When ticking that box, I can automatically add the "New namespace" to the "Old namespace" list.

@Rdornier
Copy link

The extra run could currently be avoided by giving to "Old Namespace" the two namespaces as input:
openmicroscopy.org/omero/client/mapAnnotation, A_namespace

Ok , it works !

But ticking "create new and merge" seems like an explicit agreement to merge all key-value pairs into a single A_namespace. When ticking that box, I can automatically add the "New namespace" to the "Old namespace" list.

Yes, checking the box to bypass the need of writing both namespaces would be very good to have.
Thanks !

@Tom-TBT
Copy link
Author

Tom-TBT commented Jul 16, 2024

@Rdornier I included both of your suggestions. You can go ahead and try them out with your users. Let me know if that solves your issues.

@Rdornier
Copy link

@Tom-TBT Thank you very much for your changes and for your reactivity ; I've just tested them and it works perfectly on my side ! I'm pretty sure it will also work for the users.

@imagesc-bot
Copy link

This pull request has been mentioned on Image.sc Forum. There might be relevant details there:

https://forum.image.sc/t/omero-export-list-of-file-names-from-a-project-dataset/102118/2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants