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

Update in-place experiment writing to use the new WriterContract API in slicedimage 4.0.0 #1447

Merged
merged 1 commit into from
Jul 16, 2019

Conversation

ttung
Copy link
Collaborator

@ttung ttung commented Jul 12, 2019

Test plan: in-place experiment writing tests pass.

Note that travis won't succeed until spacetx/slicedimage#109 is approved and released.

@ttung ttung requested a review from shanaxel42 July 12, 2019 18:12
@ttung ttung force-pushed the tonytung-new-inplace branch from 53c97aa to 800c651 Compare July 12, 2019 18:17
@njmei
Copy link
Collaborator

njmei commented Jul 12, 2019

I get an ImportError with the tonytung-new-inplace branch and slicedimage==4.0.0 (tonytung-url branch):

File "/home/nicholas.mei/miniconda3/envs/mfish_pipeline/lib/python3.7/site-packages/starfish/core/experiment/experiment.py", line 21, in <module>
   from slicedimage.urlpath import pathjoin
ModuleNotFoundError: No module named 'slicedimage.urlpath'

@ttung
Copy link
Collaborator Author

ttung commented Jul 12, 2019

@njmei I made a mistake in an earlier refactor. spacetx/slicedimage#110


Please note that enabling in-place experiment construction should be only done in an isolated script
dedicated to constructing an experiment, as it modifies some existing code paths.
1. Call write_experiment_json with writer_contract=InplaceWriterContract.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should be:

writer_contract=InplaceWriterContract()

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Updated.

if fov_path_generator is not None or tile_opener is not None:
warnings.warn(
"`fov_path_generator` and `tile_opener` options for writing experiment files is "
"deprecated. Use `riter_contract` instead.",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
"deprecated. Use `riter_contract` instead.",
"deprecated. Use `writer_contract` instead.",

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed!

@ttung ttung force-pushed the tonytung-new-inplace branch from 800c651 to 6411db3 Compare July 15, 2019 22:26
@codecov-io
Copy link

codecov-io commented Jul 15, 2019

Codecov Report

Merging #1447 into master will decrease coverage by 0.07%.
The diff coverage is 78.94%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #1447      +/-   ##
=========================================
- Coverage   89.78%   89.7%   -0.08%     
=========================================
  Files         161     161              
  Lines        5972    5956      -16     
=========================================
- Hits         5362    5343      -19     
- Misses        610     613       +3
Impacted Files Coverage Δ
...sh/core/experiment/builder/structured_formatter.py 88.02% <100%> (-0.09%) ⬇️
starfish/core/experiment/builder/__init__.py 96.47% <40%> (-3.53%) ⬇️
starfish/core/experiment/builder/inplace.py 84.61% <90%> (-4.58%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dc502e7...e464306. Read the comment docs.

@ttung ttung force-pushed the tonytung-new-inplace branch 2 times, most recently from 218d64e to b12514a Compare July 16, 2019 14:37
…in slicedimage 4.0.0

Test plan: in-place experiment writing tests pass.
@ttung ttung force-pushed the tonytung-new-inplace branch from b12514a to e464306 Compare July 16, 2019 17:14
@ttung ttung merged commit 593a688 into master Jul 16, 2019
@ttung ttung deleted the tonytung-new-inplace branch July 16, 2019 18:10
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.

4 participants