Open Tom-TBT opened 6 months ago
This pull request has been mentioned on Image.sc Forum. There might be relevant details there:
https://forum.image.sc/t/feedback-on-key-value-pair-scripts-enhancement/91814/1
@Tom-TBT I wonder if you could fix the merge conflicts (merge develop branch into yours)?
I just tried the Convert_KeyVal_namespace.py
and it works well.
One question I have is whether it might be preferable to update the namespace on existing annotations instead of deleting them and creating new ones? By updating existing ones, you preserve the history (don't change the 'Linked on' date) and it just feels more appropriate for this change.
Just tried modifying the script to do this, which allows you to simplify the code a bit too (see diff):
OK for the change to "Convert KV namespace". Its one advantage was to merge key-value pairs with the same namespace into a new key-value pair. What do you think, should it be a fifth script or an option of the convert script? I remember someone, maybe Petr, mentioning that merging key-value pairs was also a desired functionality.
I'll merge develop into my branch too
This pull request has been mentioned on Image.sc Forum. There might be relevant details there:
https://forum.image.sc/t/practical-differences-between-omero-tables-and-key-value-pairs/94180/4
Hi @will-moore, I'm taking care of the file attachement issue today. I did the test and like you said, the FileAnnotation remains on the server, no matter if it's attached or not.
Looking at it closely, I also noticed that I should use a better namespace.
I saw that other scripts use such NS for their FileAnnotation: openmicroscopy.org/omero/scripts/results/created/omero/export_scripts/Batch_Image_Export
Is it ok if I use the following two from now on:
openmicroscopy.org/omero/scripts/results/created/omero/annotation_scripts/Import_from_CSV
openmicroscopy.org/omero/scripts/results/created/omero/annotation_scripts/Export_to_CSV
@will-moore Could you please trigger the workflow? I have tests for all four scripts, and flake8 should be fine this time.
(done)
@will-moore please approve the workflow. I adjusted the script to your suggestions. Thanks for the review
Hello @will-moore @jburel, is there anything left blocking the merge of this PR?
Cheers
ping @pwalczysko
I was thinking we could use this on our next outreach cc @jburel
@Tom-TBT you should add yours and @JensWendt institution e.g. Gerbi when indicating your name
I added our institute affiliations
Anything else from you @jburel? Sorry for being pushy, I'm trying to close that topic.
One heads up in terms of versioning: the removal or renaming of scripts in my mind would count as a breaking change.
So it would mean a v5.9.0
tag once this is merged?
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?
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
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.
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:
Thanks a lot for the feedback. That shouldn't be too late
I was not aware that such magic was possible for Excel
I didn't know neither until today... I found this on stackoverflow
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.
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.
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 !
@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.
@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.
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 fileExport_to_csv.py
: Export key-value pairs and tags to a CSV fileRemove_KeyVal.py
: Delete key-value pairsConvert_KeyVal_namespace.py
: Changes the namespace of key-value pairsFeel 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/