search

BICCN / cell-locator

manually align specimens to annotated 3D spaces
https://cell-locator.readthedocs.io
Other
19 stars 7 forks source link

Enable preliminary LIMS support #128

Closed allemangD closed 4 years ago

allemangD commented 4 years ago

Work for issue #93; tested against a mock LIMS server KitwareMedical/AllenInstituteMockLIMS.

There is an associated commit in the KitwareMedical Slicer fork: KitwareMedical/Slicer#9b00dc5.

These add two command-line arguments to CellLocator: --lims-specimen-id and --lims-base-url. Both must be provided for LIMS functionality to work.

If both arguments are provided, CellLocator will query the LIMS server at --lims-base-url for the cell locations of specimen --lims-specimen-id and open them as a file. A new button will be enabled, Save to LIMS, which will push the changes to the LIMS server.

Currently ISVCC cell count is always assumed to be 1, since CellLocator does not yet support multiple markups on the same specimen.

allemangD commented 4 years ago

I'm concerned this was the wrong way to handle things, but I'm not sure of what I should have done in retrospect. I had made the change to Slicer with the wrong commit message, omitting the [cell-locator] prefix. To correct this, I did the following steps:

  1. Check out the allemangD/Slicer lims branch
  2. Execute git commit --amend and modify the commit message to include the prefix
  3. Execute git push --force-with-lease to save the changes.
  4. Check out the KitwareMedical/Slicer cell-locator-v4.11.0-2018-12-19-0dc589ee5 branch
  5. Modify the CMakeLists to point to the new hash (daf04f8 instead of 41b4293)
  6. Execute git commit --amend and leave the commit message unmodified, since the last commit on this branch was to update the CMakeLists to the old hash.
  7. Execute git push --force-with-lease to save the change. This step is, in retrospect, the most concerning to me since it was not just a change to a fork.

@jcfr Are there any extra considerations that should be made after doing this? What would be a better course of action if I make a bad push like this again?

jcfr commented 4 years ago

Commit https://github.com/KitwareMedical/Slicer/commit/daf04f814220bed1a15d06f14579970a0e3fb105 published on KitwareMedical/Slicer@cell-locator-v4.11.0-2018-12-19-0dc589ee5 branch makes sense :+1:

the wrong way to handle things

To update the commit message and re-publish the branch, I usually do the following:

# Assumption:
# * the current branch is cell-locator-v4.11.0-2018-12-19-0dc589ee5
# * remote called "origin" is KitwareMedical/Slicer

git commit --amend
# Fix message
git push origin cell-locator-v4.11.0-2018-12-19-0dc589ee5 --force

You will see that I explicitly reference the name of the remote and the name of the branch. I usually try to avoid general command git push without specifying remote and branch.

(I also just learned about the --force-with-lease option and would need to read more about it)

This step is, in retrospect, the most concerning to me since it was not just a change to a fork.

Would you have a complete log of what happen ?

jcfr commented 4 years ago

I suggest you squash all commit together and have a commit message like the following:

ENH: Add LIMS support (See #93)

This commit updates Slicer adding support for flags `--specimen-id` and `--lims-url`.
If `--specimen-id` is provided, the IVSCC cell locations for that specimen id is displayed
at XXX and it enables the "Save to LIMS" button to write cell location changes back
to that specimen id.

IVSCC expected cell count is currently unsupported.

List of Slicer changes:
$ git shortlog 0661948e5..daf04f814 --no-merges
David Allemang (1):
      [cell-locator] ENH: Add command line args --specimen-id and --lims-url for LIMS integration.

Before doing so consider also comment added to https://github.com/KitwareMedical/Slicer/commit/daf04f814220bed1a15d06f14579970a0e3fb105

allemangD commented 4 years ago

To update the commit message and re-publish the branch, I usually do the following:

# Assumption:
# * the current branch is cell-locator-v4.11.0-2018-12-19-0dc589ee5
# * remote called "origin" is KitwareMedical/Slicer

git commit --amend
# Fix message
git push origin cell-locator-v4.11.0-2018-12-19-0dc589ee5 --force

You will see that I explicitly reference the name of the remote and the name of the branch. I usually try to avoid general command git push without specifying remote and branch.

This all makes sense. Thanks. After further reading about using --force/--force-with-lease I'd seen that this is almost universally advised against in public repositories, so I was concerned there may be some unforeseen issue with CI or the build system.

I think I will also try to get in the habit of explicitly specifying the remote, especially if I ever need to force a push. Thanks for your advice.

I suggest you squash all commit together and have a commit message like the following:

Before doing so consider also comment added to KitwareMedical/Slicer@daf04f8

I'll also update the documentation in this PR and in the issue to specify the new flag names.

allemangD commented 4 years ago

All the above issues have been addressed. The changes in each branch have been squashed into single commits, and the CLI arguments have been renamed lims-specimen-id and lims-base-url. References to those names in the PR description and in commit messages have also been updated.

jcfr commented 4 years ago

Well done

In the commit message, I suggest you apply the following changes:

From 

ENH: Add LIMS support (See #93) 

This commit updates Slicer adding support for flags `--lims-specimen-id` and `--lims-base-url`.

If `--lims-specimen-id` is provided, the IVSCC cell locations for that specimen id is displayed
at XXX and it enables the "Save to LIMS" button to write cell location changes back
to that specimen id.

IVSCC expected cell count is currently unsupported.

List of Slicer changes:

$ git shortlog 0661948e5..9b00dc521f --no-merges
David Allemang (1):
      [cell-locator] ENH: Add command line args --lims-specimen-id and --lims-base-url for LIMS integration.

into 

ENH: Add LIMS support (See #93) 

This commit updates CellLocator and Slicer adding support for flags `--lims-specimen-id`
and `--lims-base-url`.

If `--lims-specimen-id` is provided, it is used:
* At application startup time to 
   (1) retrieve the corresponding annotation from LIMS and load it by using the `/specimen_metadata/view` endpoint.
   (2) enable the "Upload Annotation" button
* After user initiate annotation upload to LIMS while using the `/specimen_metadata/store` endpoint.

The "kind" parameter for both  endpoints `/specimen_metadata/view` and `/specimen_metadata/store`
is set to "IVSCC cell locations"

IVSCC expected cell count is currently unsupported.

For testing this functionality, a mock server as been implemented. Relevant instructions 
are available here: https://github.com/KitwareMedical/AllenInstituteMockLIMS

List of Slicer changes:

$ git shortlog 0661948e5..9b00dc521f --no-merges
David Allemang (1):
      [cell-locator] ENH: Add command line args --lims-specimen-id and --lims-base-url for LIMS integration.
jcfr commented 4 years ago

Thanks for updating the topic, once I have tested locally, I will integrate and generate packages.