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

Change behaviour of dtype on equivalent sources #516

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

Conversation

santisoler
Copy link
Member

@santisoler santisoler commented Jun 18, 2024

Make use of the dtype argument in equivalent sources only to build the jacobian matrix. Stop using the dtype for casting predictions, coordinates and location of the sources. Predicted arrays will by float64, while coordinates and location of the sources will have the same types as the original arguments.

Apply `dtype` argument only to the jacobian matrix and the predictions
for equivalent sources. Don't use the `dtype` for casting coordinates
and location of the sources.
@leouieda
Copy link
Member

Hi @santisoler, I'm curious about why this is needed.

@santisoler
Copy link
Member Author

Hi @leouieda. I didn't quite like that the docstring for dtype weren't reflecting the true behaviour of it. We were using dtype not only for the jacobian and the predictions, but for casting the coordinates of data and points as well. I noticed this because some tests started to fail on Numpy 2.0 (I think because they changed some casting rules when operating with two different type of floats) and the reason for it was because we weren't casting the coordinates of sources every time we created them (see #514).

I fixed that issue in #514 by converting the coordinates of the sources, but it sounded wrong: we were using dtype in a different way as documented. What do you think?

@leouieda
Copy link
Member

As long as the Jacobian and predictions are still in the correct dtype then it makes sense. Otherwise, it would be better to fix the docs, right?

@santisoler
Copy link
Member Author

I think I'd prefer to keep things simple. The main reason to include the dtype is to lower the memory requirements to fit the sources, so the most important place to use it is for building the Jacobian. I don't think there's too much sense to use it to cast the coordinates of the sources.

And now that I think about it, since we don't use the Jacobian to make predictions, once the coefficients are fit, we can just return predictions as float64. Is there any sense to keep the predictions as float32 as well?

@leouieda
Copy link
Member

I'm not sure. I suppose I thought of the dtype being for the model so then predictions would also be float32. I guess having the dtype impact only the jacobian and not the output of the class feels wrong.

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