Tem with install dependancies #47

arjunsingh3600 · 2021-12-16T01:24:05Z

No description provided.

…th_config yaml. added local sim path

arjunsingh3600 · 2021-12-16T01:51:18Z

This branch contains the remaining implementation for the tem wrapper (along with some tweaks to existing methods). The methods added in the PR require a local installation for tem wrapper and hence the associated unit tests will fail.

arjunsingh3600 · 2021-12-16T02:19:49Z

@jedyeo @thisTyler @calhep

geoffwoollard · 2021-12-16T07:10:11Z

Good work guys. I suggest doing the containerization ASAP, and then merging this after.

ninamiolane

Very nice! Nothing to say code-wise 💯

Design-wise, the two functions:

run
get_image_data
are misleading for an external user, because:
their main (computationally expensive) objective is to run the simulator, yet only one of these functions is called run,
the keyword get_ is usually used to denote a function/method that returns an object (.e.g image_data) that has already been computed. It is misleading to name a function get_ and actually have this function do a real big computation.

I would suggest the following design:

def run with a parameter denoting in which format we want the output of the simulation (.mrc? micrograph? particles?)
-def _run_executable only containing the beginning of get_image_data
such that:
the core work is done by the executable
the format conversions of inputs/outputs are done in the run function.

But we can merge this PR first, and address the design's questions next week when I review the whole codebase 🎉

Congrats again, great work!

ninamiolane

Actually, requesting changes since we need the tests to pass :/ I am assuming that this is passing with your local installation of the TEM, but not passing with the GitHub actions.

fredericpoitevin · 2022-01-20T18:38:21Z

Actually, requesting changes since we need the tests to pass :/ I am assuming that this is passing with your local installation of the TEM, but not passing with the GitHub actions.

Yes I'm hoping to get to that by end of week...

fredericpoitevin · 2022-01-24T01:32:11Z

Hi @arjunsingh3600 ! Now that #50 has been merged into master, we're merging master here which will relaunch the checks. Let's see how that goes! I'll start reviewing once the checks pass.

…pendancies

codecov · 2022-01-24T02:04:54Z

Codecov Report

Merging #47 (678d83b) into master (ea3faaf) will not change coverage.
The diff coverage is 86.96%.

@@           Coverage Diff           @@
##           master      #47   +/-   ##
=======================================
  Coverage   97.20%   97.20%           
=======================================
  Files           8        8           
  Lines         285      285           
=======================================
  Hits          277      277           
  Misses          8        8

Impacted Files	Coverage Δ
simSPI/tem.py	`93.83% <86.96%> (ø)`

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 ea3faaf...678d83b. Read the comment docs.

…oint is not working in Singularity, we need to provide the full path to the TEM-simulator executable.

fredericpoitevin · 2022-01-24T16:32:01Z

@arjunsingh3600 thanks for looking into it earlier, you were on the right track! Hope you don't mind if I take over - should be done soon.

fredericpoitevin · 2022-01-24T17:17:38Z

Hmmm I spoke too fast, I don't really understand why this (https://github.com/compSPI/simSPI/runs/4924673073?check_suite_focus=true) is failing:

=================================== FAILURES ===================================
_____________________________ test_get_image_data ______________________________

sample_class = <simSPI.tem.TEMSimulator object at 0x7f3713372fa0>

    def test_get_image_data(sample_class):
        """Test whether mrc data is generated from local tem installation.
    
        Notes
        -----
        This test requires a local TEM sim installation to run.
        """
        sample_class.write_inp_file()
>       data = sample_class.get_image_data()

tests/test_tem.py:259: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
simSPI/tem.py:286: in get_image_data
    data = io.mrc2data(self.output_path_dict["mrc_file"])
/opt/anaconda/lib/python3.9/site-packages/ioSPI/cryoemio.py:19: in mrc2data
    with mrcfile.open(mrc_file, "r", permissive=True) as mrc:
/opt/anaconda/lib/python3.9/site-packages/mrcfile/load_functions.py:138: in open
    return NewMrc(name, mode=mode, permissive=permissive,
/opt/anaconda/lib/python3.9/site-packages/mrcfile/mrcfile.py:108: in __init__
    self._open_file(name)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

self = <[AttributeError("'NoneType' object has no attribute 'name'") raised in repr()] MrcFile object at 0x7f3713372df0>
name = '/tmp/pytest-of-runner/pytest-0/test_get_image_data0/_randomrot.mrc'

    def _open_file(self, name):
        """Open a file object to use as the I/O stream."""
>       self._iostream = open(name, self._mode + 'b')
E       FileNotFoundError: [Errno 2] No such file or directory: '/tmp/pytest-of-runner/pytest-0/test_get_image_data0/_randomrot.mrc'

/opt/anaconda/lib/python3.9/site-packages/mrcfile/mrcfile.py:125: FileNotFoundError
----------------------------- Captured stdout call -----------------------------
Failed to create log file.
Could not open file test_files/_randomrot.txt for reading.
___________________________________ test_run ___________________________________

sample_class = <simSPI.tem.TEMSimulator object at 0x7f3726bb4d30>

    def test_run(sample_class):
        """Test whether run returns and exports particles with expected shape.
    
        Notes
        -----
        This test requires a local TEM sim installation to run.
        """
>       particles = sample_class.run(export_particles=True)

tests/test_tem.py:272: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
simSPI/tem.py:217: in run
    micrograph_data = self.get_image_data()
simSPI/tem.py:286: in get_image_data
    data = io.mrc2data(self.output_path_dict["mrc_file"])
/opt/anaconda/lib/python3.9/site-packages/ioSPI/cryoemio.py:19: in mrc2data
    with mrcfile.open(mrc_file, "r", permissive=True) as mrc:
/opt/anaconda/lib/python3.9/site-packages/mrcfile/load_functions.py:138: in open
    return NewMrc(name, mode=mode, permissive=permissive,
/opt/anaconda/lib/python3.9/site-packages/mrcfile/mrcfile.py:108: in __init__
    self._open_file(name)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

self = <[AttributeError("'NoneType' object has no attribute 'name'") raised in repr()] MrcFile object at 0x7f37138cb100>
name = '/tmp/pytest-of-runner/pytest-0/test_run0/_randomrot.mrc'

    def _open_file(self, name):
        """Open a file object to use as the I/O stream."""
>       self._iostream = open(name, self._mode + 'b')
E       FileNotFoundError: [Errno 2] No such file or directory: '/tmp/pytest-of-runner/pytest-0/test_run0/_randomrot.mrc'

/opt/anaconda/lib/python3.9/site-packages/mrcfile/mrcfile.py:125: FileNotFoundError
----------------------------- Captured stdout call -----------------------------
Failed to create log file.
Could not open file test_files/_randomrot.txt for reading.

I tried on SDF and the tests pass:

[fpoitevi@sdf-login03 simSPI]$ singularity exec /sdf/scratch/fpoitevi/singularity_images/simspi_entrypoint.sif /bin/bash /opt/entrypoint.sh pytest --cov-report term --cov-report xml:coverage.xml --cov=/work/simSPI /work/tests
=================================================================== test session starts ===================================================================
platform linux -- Python 3.9.9, pytest-6.2.5, py-1.11.0, pluggy-1.0.0
rootdir: /work
plugins: cov-3.0.0
collected 27 items                                                                                                                                        

../../../../../work/tests/test_crd.py ..                                                                                                            [  7%]
../../../../../work/tests/test_fov.py .......                                                                                                       [ 33%]
../../../../../work/tests/test_multislice.py .....                                                                                                  [ 51%]
../../../../../work/tests/test_noise.py .                                                                                                           [ 55%]
../../../../../work/tests/test_shift.py .                                                                                                           [ 59%]
../../../../../work/tests/test_tem.py ........                                                                                                      [ 88%]
../../../../../work/tests/test_transfer.py ...                                                                                                      [100%]


----------- coverage: platform linux, python 3.9.9-final-0 -----------
Name                                           Stmts   Miss  Cover   Missing
----------------------------------------------------------------------------
/work/simSPI/__init__.py                           1      0   100%
/work/simSPI/crd.py                               24      0   100%
/work/simSPI/fov.py                               76      0   100%
/work/simSPI/linear_simulator/noise_utils.py       8      0   100%
/work/simSPI/linear_simulator/shift_utils.py      22      0   100%
/work/simSPI/multislice.py                        21      0   100%
/work/simSPI/tem.py                               81      5    94%   265, 269-270, 273, 302
/work/simSPI/transfer.py                          52      3    94%   101, 108, 178
----------------------------------------------------------------------------
TOTAL                                            285      8    97%
Coverage XML written to file coverage.xml

Required test coverage of 90.0% reached. Total coverage: 97.19%
======================================================= 27 passed, 21 warnings in 70.71s (0:01:10) ========================================================

I am tempted to remove the workflow that tests in Singularity container: @ninamiolane would you be OK with that? As long as the tests pass in the Docker container, we know the code works - and it's up to the user to figure out things with Singularity.

…ests pass in Docker containers is proof enough and streamlines checks.

fredericpoitevin · 2022-01-24T17:32:30Z

OK, we're good for review now! @ninamiolane @geoffwoollard

fredericpoitevin

LGTM!

fredericpoitevin · 2022-01-24T17:35:47Z

simSPI/tem.py

+        kwargs
+            Arbitrary keyword arguments.
+


Do we need this? I don't see them passed in the function?

fredericpoitevin · 2022-01-24T17:36:42Z

simSPI/tem.py

+        pad : double, (default = 5)
+            Pad to be added to maximal dimension of the object read from pdb_file


Is it in pixel or physical (Angstrom) unit?

ninamiolane · 2022-01-24T18:23:10Z

.github/workflows/test_singularity.yml

@@ -2,7 +2,7 @@ name: "Linting and Testing (Singularity)"

 on:
  push:
-    branches: [temsim-test-in-container]
+    branches: [simspi-test-singularity]


should this be called test-container? (I thought we removed singularity?) we also do not need simspi, since it is on the simSPI repo.

ah good point!

I kept the test_singularity workflow but it's only triggered if we push on this branch.

So maybe in terms of renaming:

test_singularity.yml (keep name): rename trigger branch to test-singularity

test_docker.yml: I will split it in a way similar to what you did on ioSPI once we merge this branch (linting decoupled from testing). The trigger branch will be rename test-container.

ninamiolane

LGTM! I made comments on the design of the run and get_image_data functions on another PR: it would be good to have a look at them to clean the architecture of the functions within the codebase.

fredericpoitevin · 2022-01-24T19:52:19Z

I'll merge now and do the linting/testing decoupling in a different PR.

arjunsingh3600 added 5 commits December 15, 2021 15:22

Added run, get_image_data outline. reworked generate_path_dict and pa…

4be05f3

…th_config yaml. added local sim path

implemented run and get_image_data. linting

db5a2b3

updated tests

bacad56

deep sense fixes

b94bd28

test fixes

0c5b16b

replaced os with subprocess

4cc0b75

arjunsingh3600 requested review from fredericpoitevin, geoffwoollard and ninamiolane December 16, 2021 02:19

fredericpoitevin mentioned this pull request Dec 16, 2021

Run TEM simulator tests in container. #48

Closed

ninamiolane approved these changes Jan 20, 2022

View reviewed changes

ninamiolane requested changes Jan 20, 2022

View reviewed changes

fredericpoitevin linked an issue Jan 24, 2022 that may be closed by this pull request

Run TEM simulator tests in container. #48

Closed

Merge remote-tracking branch 'origin/master' into tem_with_install_de…

6554ba9

…pendancies

arjunsingh3600 and others added 4 commits January 23, 2022 23:49

tem tests / deepsense fix- dictionary formatting

ed7b3fc

changed tem sim path in tests for container

e14c4c0

changed tem sim path to './TEM-simulator'

7447bcf

For some reason, the PATH environment variable exported in the entryp…

1182ce0

…oint is not working in Singularity, we need to provide the full path to the TEM-simulator executable.

Removing requirement to check test in Singularity container. Having t…

678d83b

…ests pass in Docker containers is proof enough and streamlines checks.

fredericpoitevin requested a review from ninamiolane January 24, 2022 17:32

fredericpoitevin approved these changes Jan 24, 2022

View reviewed changes

ninamiolane reviewed Jan 24, 2022

View reviewed changes

ninamiolane approved these changes Jan 24, 2022

View reviewed changes

fredericpoitevin merged commit dfa5157 into master Jan 24, 2022

harshit-gupta-cor mentioned this pull request Jan 26, 2022

projector module with the right dependencies #53

Merged

fredericpoitevin deleted the tem_with_install_dependancies branch January 29, 2022 23:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tem with install dependancies #47

Tem with install dependancies #47

arjunsingh3600 commented Dec 16, 2021

arjunsingh3600 commented Dec 16, 2021

arjunsingh3600 commented Dec 16, 2021

geoffwoollard commented Dec 16, 2021

ninamiolane left a comment

ninamiolane left a comment

fredericpoitevin commented Jan 20, 2022

fredericpoitevin commented Jan 24, 2022 •

edited

Loading

codecov bot commented Jan 24, 2022 •

edited

Loading

fredericpoitevin commented Jan 24, 2022

fredericpoitevin commented Jan 24, 2022

fredericpoitevin commented Jan 24, 2022

fredericpoitevin left a comment

fredericpoitevin Jan 24, 2022

fredericpoitevin Jan 24, 2022

ninamiolane Jan 24, 2022

fredericpoitevin Jan 24, 2022

ninamiolane left a comment

fredericpoitevin commented Jan 24, 2022

		pad : double, (default = 5)
		Pad to be added to maximal dimension of the object read from pdb_file

Tem with install dependancies #47

Tem with install dependancies #47

Conversation

arjunsingh3600 commented Dec 16, 2021

arjunsingh3600 commented Dec 16, 2021

arjunsingh3600 commented Dec 16, 2021

geoffwoollard commented Dec 16, 2021

ninamiolane left a comment

Choose a reason for hiding this comment

ninamiolane left a comment

Choose a reason for hiding this comment

fredericpoitevin commented Jan 20, 2022

fredericpoitevin commented Jan 24, 2022 • edited Loading

codecov bot commented Jan 24, 2022 • edited Loading

Codecov Report

fredericpoitevin commented Jan 24, 2022

fredericpoitevin commented Jan 24, 2022

fredericpoitevin commented Jan 24, 2022

fredericpoitevin left a comment

Choose a reason for hiding this comment

fredericpoitevin Jan 24, 2022

Choose a reason for hiding this comment

fredericpoitevin Jan 24, 2022

Choose a reason for hiding this comment

ninamiolane Jan 24, 2022

Choose a reason for hiding this comment

fredericpoitevin Jan 24, 2022

Choose a reason for hiding this comment

ninamiolane left a comment

Choose a reason for hiding this comment

fredericpoitevin commented Jan 24, 2022

fredericpoitevin commented Jan 24, 2022 •

edited

Loading

codecov bot commented Jan 24, 2022 •

edited

Loading