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

A few unrelated bugfixes #15

Merged
merged 4 commits into from
Feb 6, 2023
Merged

Conversation

keflavich
Copy link
Contributor

If you want, I can split these up. The most substantive changes are re: #14. Others are things I put in to help me track the progress of the code; these could be removed. There's also an import change (that's a real bugfix) and a change to the packagename in the setup.

@andrew-saydjari
Copy link
Collaborator

I will let @schlafly do the review, but there is some problem associated with versioning in conda that has caused issues with the conda-forge feedstock for crowdsource that is causing my CloudCovErr.jl unit tests to fail, so I releasing a new crowdsource package was on my to do list anyway. Happy to handle that part of it.

@schlafly
Copy link
Owner

Thanks for the PR. Let me review and merge tomorrow. I may tweak the get_sizes(...) implementation, but it's good to have that configurable now. I don't understand python packaging well enough; Andrew, can you confirm the change to the name in the setup script? Thanks!

@schlafly
Copy link
Owner

Can you see if this API for get_sizes(...) fits your needs? I also took the liberty of adding some line breaks. I haven't been being careful about print(..., flush=True); if you're having issues there, you might consider starting python in unbuffered mode (python -u), rather than adding to the print messages.

@keflavich
Copy link
Contributor Author

the flush=True stuff probably was unnecessary.

I think the API works but I'm not sure I know what blistsz is... is blist a list of bright stars?

In any case, yeah, I think this adds the flexibility needed.

@schlafly
Copy link
Owner

Yes, in DECaPS / WISE we occasionally observe alpha Cen / whatever and it's possible for hundreds of pixels to be masked. We try to do a little better in those cases by using the full 299x299 stamp we have. That's only new in that I moved the hard coded constant into a default parameter.

@andrew-saydjari
Copy link
Collaborator

@schlafly Looks good to me. I built and tested on TestPyPI and had no issues. I just bumped the patch version number. After you merge, I will issue a Github release, push to PyPi production, after which it should appear on conda-forge same day. My real concern is that I made CloudCovErr.jl depend on crowdsource (a bad idea in retrospect) and I am worried about downstream compatibility issues. So I am fine with going ahead with this as long as you are ok with some possible patch version number incrementing while I troubleshoot that in the next few days.

@schlafly
Copy link
Owner

schlafly commented Feb 6, 2023

Great, merging.

@schlafly schlafly merged commit dcfddc7 into schlafly:master Feb 6, 2023
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

3 participants