search

compSPI / ioSPI

I/O and Data Visualization
MIT License
4 stars 7 forks source link

Cryoemio ctf #40

Closed arjunsingh3600 closed 2 years ago

arjunsingh3600 commented 2 years ago

changes to io functions for simSPI to add support for ctf parameters

jedyeo commented 2 years ago

This is ready for review but this is a tricky merge conflict :( @geoffwoollard

ninamiolane commented 2 years ago

The merge conflict is happening because this file is not called cryoemio anymore? Maybe use the correct new name for this file and try again?

Looking good otherwise.

ninamiolane commented 2 years ago

@geoffwoollard The merge conflict is happening because this file is not called cryoemio anymore? Maybe use the correct new name for this file and try again?

review-notebook-app[bot] commented 2 years ago

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

codecov[bot] commented 2 years ago

Codecov Report

Merging #40 (cbef541) into master (295b6b5) will not change coverage. The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #40   +/-   ##
=======================================
  Coverage   98.41%   98.41%           
=======================================
  Files           5        5           
  Lines         188      188           
=======================================
  Hits          185      185           
  Misses          3        3
Impacted Files Coverage Δ
__init__.py 100.00% <ø> (ø)
ioSPI/__init__.py

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 2eddb5e...cbef541. Read the comment docs.

jedyeo commented 2 years ago

@ninamiolane are we good to merge this?

jedyeo commented 2 years ago

@ninamiolane i think i fixed it, but don't want to merge without your approval :')

jedyeo commented 2 years ago

@ninamiolane think everything checks out!

geoffwoollard commented 2 years ago

We're going ahead with this PR? It has tem simulator specifics in it? The discussion at https://github.com/compSPI/ioSPI/pull/25 suggested that this would be moved to simSPI.

jedyeo commented 2 years ago

We're going ahead with this PR? It has tem simulator specifics in it? The discussion at #25 suggested that this would be moved to simSPI.

See https://github.com/compSPI/ioSPI/pull/25#issuecomment-1040634866

I think this should be good to merge, tem specifics have been removed.

geoffwoollard commented 2 years ago

I don't understand this PR:

  • it is almost only bringing back files that should be deleted (see previous PRs): is this a GitHub error?
  • it is only dealing with imports (..ioSPI): why the change? and that does not match the title of the PR...? Nothing on the CTF?

Those are all good points @ninamiolane - I am confused too. @jedyeo is Nina right? Or perhaps something was lost in a commit?

Also, fourier isn't inioSPIanymore.... this wouldn't pass tests anymore. See https://github.com/compSPI/ioSPI/pull/49

jedyeo commented 2 years ago

@ninamiolane @geoffwoollard after discussing with @arjunsingh3600 it seems that #46 has made this PR redundant (i.e. the refactoring that took place pulled out many of the changes that we made in this branch), but before I'm comfortable with closing this, I want to touch base with @thisTyler early tomorrow and just see that we're all on the same page.

thisFreya commented 2 years ago

@ninamiolane @geoffwoollard Closing this PR as it has been messed up by refactoring quite significantly - we will remake the changes that this originally had in a fresh branch.