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

Prep for release of v5.1.0 #8870

Merged
merged 17 commits into from
May 10, 2024
Merged

Prep for release of v5.1.0 #8870

merged 17 commits into from
May 10, 2024

Conversation

dcollins15
Copy link
Contributor

@dcollins15 dcollins15 commented May 8, 2024

Final changes before releasing v5.1.0

ToDo:

  • Update DESCRIPTION
  • Update NEWS
  • Update cran-comments
  • Check dates

@dcollins15 dcollins15 requested a review from Gesmira May 8, 2024 16:47
@dcollins15 dcollins15 force-pushed the release/5.1.0 branch 2 times, most recently from 193e7b9 to 408320a Compare May 8, 2024 21:52
@dcollins15
Copy link
Contributor Author

Alright so, it hadn't occurred to me that the new files I added to test Visium HD loaders dramatically increase the size of the package tarball (duh 🤦). Initially, the package was clocking in at a whopping ~120MB but I've managed to get it down to ~8MB by tidying things up. The only loss in test coverage is in reading tissue_hires_image.png files since these files are so large.

The tarball for v5.0.3 was on ~2MB so I'd like to try to shrink this bad boy down a little more before I force users to start pulling it down from CRAN.

While I was thinking about these tests I realized I'd also been overusing the skip_on_cran function by including it in tests depending on packages from Suggests - it probably should've been obvious but we only need to skip tests that rely on packages from BioConductor.

@dcollins15
Copy link
Contributor Author

Ok, I think I've found a much better solution - replacing the tissue_{hires/lowres}_image.png files with empty images of the same dimensions - at ~...MB each the hires images in particular were blowing up the size of the final package. Now, the tarball is back down to ~2MB, if anything it's slightly smaller than it was for v5.0.3 🙌

Apologies for all the force-pushing I've been doing, I ended up doing more faffing about here than I intended to and I get a little neurotic about keeping the git history clean - I'm still just finishing up the last few reverse dependency checks but I think this should be good to merge now 🤠

@Gesmira
Copy link
Contributor

Gesmira commented May 9, 2024

Sounds good! Great work!

@dcollins15
Copy link
Contributor Author

Alright alright alright - now that I've finished running reverse dependency checks it looks like we're going to break AnanseSeurat with this release, well at least the way things are now.

It looks like AnanseSeurat calls the AggregateExpression method and passes in the slot parameter. AggregateExpression doesn't actually expose slot but it gets handed down to PseudobulkExpression via the ... all the same 🤦

It looks like when @mhkowalski deprecated the slot parameter for v5.0 we were initially raising a warning, but also added code to switch over to an error in v5.1 - this is what's causing AnanseSeurat to break. This gives us two options to proceed:

  1. Drop the conditional and just leave the parameter as a soft deprecation for now - we could push the warning back to v5.2 but I feel like we'll probably end up back in this same situation in a few months time
  2. Post an issue to https://github.com/JGASmits/AnanseSeurat/issues and submit to CRAN with a note - I'm not sure how much notice we are expected to give and whether or not the deprecation warning itself counts

The only other place we've deprecated the slot param in RidgePlot and here we're just making a call to soft_deprecate() - my vote would be to just to the same for PseudobulkExpression and let AnanseSeurat live to fight another day 🤕

@dcollins15 dcollins15 merged commit bba2165 into develop May 10, 2024
0 of 2 checks passed
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.

2 participants