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

fix shape returned from keplerlib.propagate #986

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Tontyna
Copy link

@Tontyna Tontyna commented Jul 23, 2024

fiixes #959

@brandon-rhodes
Copy link
Member

@JoshPaterson — I'm inclined to accept this pull request, maybe after requesting that a test be written so that a future well-meaning coder doesn't break this edge case again. But I thought I should loop you in, since I think this is your code? Simply comment here over the next few days if you'd like to weigh in. Otherwise, if I don't hear from you by maybe the middle of next week, I'll assume you're busy these days. I hope you're doing well!

@JoshPaterson
Copy link
Contributor

Hi @brandon-rhodes, I'll take a look at this. I'm wondering if instead of checking if squeezing will make the wrong shape and then using a conditional to fix it, can the squeezing be made to work correctly the first time? If either you or @Tontyna have ideas about that let me know!

@JoshPaterson
Copy link
Contributor

Perhaps this fix can be accepted to get things working and then if it can be improved that can be a separate PR.

@Tontyna
Copy link
Author

Tontyna commented Jul 29, 2024

I tried to avoid reshape, but squeeze can't do nothing about it.
The whole calculation -- atleast_ld ! -- doesn't distinguish between a given scalar time and a [time], so we must remember (and reshape).

@Tontyna
Copy link
Author

Tontyna commented Jul 29, 2024

One could do without the expected_shape variable by not modifying the t1 parameter within the function and doing the shape-check / reshape only at the end of the function.

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.

3 participants