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

packages / tagbot / badges #87

Merged
merged 9 commits into from
Jan 27, 2021
Merged

packages / tagbot / badges #87

merged 9 commits into from
Jan 27, 2021

Conversation

JeffFessler
Copy link
Owner

especially for updated MIRTio that addresses a HDF5.jl issue

@codecov
Copy link

codecov bot commented Jan 25, 2021

Codecov Report

Merging #87 (6c98680) into master (7c2a170) will increase coverage by 0.00%.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #87   +/-   ##
=======================================
  Coverage   99.75%   99.75%           
=======================================
  Files          49       49           
  Lines        2409     2411    +2     
=======================================
+ Hits         2403     2405    +2     
  Misses          6        6           
Impacted Files Coverage Δ
src/fbp/ellipse_im.jl 100.00% <0.00%> (ø)
src/mri/sensemap-sim.jl 100.00% <0.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 7c2a170...6c98680. Read the comment docs.

Copy link
Contributor

@johnnychen94 johnnychen94 left a comment

Choose a reason for hiding this comment

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

It's very possible that other (old) packages aren't compatible to FillArrays 0.11 and an over-restricted version compat can result in a very unpleasant experience during version resolving. For this reason, it's not advisable to drop old versions too eagerly.

Dropping old version often due to the following reasons:

  • the feature we need only exists in the new version, and
  • declaring compatibility to old version requires more maintenance work

According to https://semver.org/, MIRT should be compatible to FFTW@1 and Wavelets@0.9 since it doesn't require the new features there (does it?), and there's no extra maintenance burden here to declare the compatibility(it's the responsibility of FFTW and Wavelets to maintain the compatibility). Thus I made two suggestions to revert it. Hopefully, we could choose to not drop the old versions, but I'm not familiar with the MIRT internal, so you make the choices.

Project.toml Outdated Show resolved Hide resolved
Project.toml Outdated Show resolved Hide resolved
@JeffFessler JeffFessler marked this pull request as draft January 27, 2021 03:03
@JeffFessler
Copy link
Owner Author

@johnnychen94 thanks for your advice on this. it passes the tests now (including older versions of the packages in compat) so can you please take one more look at this before i squash and merge? i am having some git push issues so i probably messed up your authorship on this one so apologies for that. my biggest question mark is whether the "Wavelets 0.9" is really the right entry given the updates we just did for Wavelets, but it seems to work so if you say it is ok then i am ready to proceed.

@JeffFessler JeffFessler marked this pull request as ready for review January 27, 2021 03:36
Copy link
Contributor

@johnnychen94 johnnychen94 left a comment

Choose a reason for hiding this comment

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

MIRT has nothing to do with the version conflict for SpecialFunctions, so as long as Wavelets updates its compat section, Pkg can automatically figure out a happy version for it. Thus I would personally revert the entire Project.toml because it fixes nothing.

It all depends on how you expect this PR does, if you intend to change the compat entries, then it's okay.

Project.toml Outdated Show resolved Hide resolved
Manifest.toml Outdated Show resolved Hide resolved
JeffFessler and others added 2 commits January 27, 2021 14:35
Co-authored-by: Johnny Chen <johnnychen94@hotmail.com>
@JeffFessler JeffFessler merged commit 12cbf0e into master Jan 27, 2021
@JeffFessler JeffFessler deleted the packs branch January 27, 2021 20:54
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.

None yet

2 participants