compSPI / simSPI

Generative Models
9 stars 10 forks source link

Multislice compSPI fft #66

Closed geoffwoollard closed 2 years ago

geoffwoollard commented 2 years ago

Addresses part of https://github.com/compSPI/ioSPI/issues/45

geoffwoollard commented 2 years ago

@ninamiolane @fredericpoitevin what’s going on in these linting errors? they pass my hooks when I commit locally… do I run black . FILE.py or black . FILE.py --check or are there some special flags. It’s seems there’s some difference between my local black and git black.

Screen Shot 2022-02-11 at 2 50 32 PM

Locally I get

(simSPI_d) dhcp-128-189-220-173:tests gw$ black . --check
All done! ✨ 🍰 ✨
11 files would be left unchanged.
fredericpoitevin commented 2 years ago

@geoffwoollard it looks like your branch was not up-to-date with current master when you started working on it, the "build" workflow is not used anymore. Try to merge master in your branch, resolve possible conflicts and push again, please. This should update this PR and trigger the new workflows. Thanks

geoffwoollard commented 2 years ago

@geoffwoollard it looks like your branch was not up-to-date with current master when you started working on it, the "build" workflow is not used anymore. Try to merge master in your branch, resolve possible conflicts and push again, please. This should update this PR and trigger the new workflows. Thanks

Oops, I thought I rebased, but this must have been in ioSPI or compSPI since they are all involved with removing the numpy fft code. OK I'll do that.

geoffwoollard commented 2 years ago

@fredericpoitevin I don't know about that. It looks like nothing changes with build

https://github.com/geoffwoollard/simSPI/compare/multislice_compspifft...compSPI:master

geoffwoollard commented 2 years ago

OK I merged in compSPI/simSPI:master and am still getting the same linting error.

Why is the linting and building not split up as it is in newer PRs?

Maybe I'll try to close this PR and open another?

codecov[bot] commented 2 years ago

Codecov Report

Merging #66 (9877fd7) into master (9c62f20) will increase coverage by 0.06%. The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #66      +/-   ##
==========================================
+ Coverage   97.80%   97.85%   +0.06%     
==========================================
  Files          10       10              
  Lines         362      372      +10     
==========================================
+ Hits          354      364      +10     
  Misses          8        8              
Impacted Files Coverage Δ
simSPI/multislice.py 100.00% <100.00%> (ø)

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 9c62f20...9877fd7. Read the comment docs.

fredericpoitevin commented 2 years ago

Oh I saw build and assumed it was the workflow name but it was already the lint workflow, sorry. Well it does not hurt to check.

fredericpoitevin commented 2 years ago

If you want to make sure you test with the same version as GitHub Actions, check .github/workflows/lint.yml And see how things are pip installed. I built a simSPI-test conda environment locally using dev-requirements.txt - maybe consider using it to lint locally?

geoffwoollard commented 2 years ago

I turned off the precommit hook, since it's black was conflicting with my local black.

Now the linting is passing on github.

Not sure why that happened, and I need to resolve it before I turn on the precommit hook again.

In anycase this PR should be good to review.

There's some back and forth pytorch and numpy things happening. But that's because the compSPI fft functions need torch tensors that are shaped (1,1,n_pixels,n_pixels)

geoffwoollard commented 2 years ago

merging now since https://github.com/compSPI/ioSPI/pull/49 broke simSPI (since simSPI used fourier in ioSPI), and this PR will fixes that.