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

ENH: Outsource apply() from transform objects #195

Merged
merged 16 commits into from May 17, 2024
Merged

Conversation

jmarabotto
Copy link
Contributor

@jmarabotto jmarabotto commented Apr 5, 2024

I have joined CHUV (Lausanne, Switzerland) for a 5 month placement and am working with Oscar Esteban on this project. We worked through this problem together as an example of how I can contribute and resolve issues, as I am somewhat new to GitHub. Eventually, this Resolves #192 - but for now, this acts as an example and further changes are needed :)

@effigies
Copy link
Member

effigies commented Apr 5, 2024

I assume this is intended to close #192? If so, please add "Resolves #192" or similar to your top post.

@oesteban
Copy link
Collaborator

Okay, with #197 out of the way we can focus on finalizing this. @effigies -- I'm working with Julien on this one, I will walk him through the process of preparing the PR we will let you know when this is ready for you to eyeball it.

@jmarabotto jmarabotto marked this pull request as ready for review May 10, 2024 09:23
Copy link
Collaborator

@oesteban oesteban left a comment

Choose a reason for hiding this comment

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

Let's finalize this today.

else None
)

for t, xfm_t in enumerate(self):
Copy link
Collaborator

Choose a reason for hiding this comment

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

I've been holding on this one - the new implementation is 4D, while the old implementation was 3D+t (meaning that you could not resample off-gried on the fourth dimension).

We should make sure we meet today @effigies, @jmarabotto and discuss this.

cc/ @sgiavasis who may be interested in learning more.

nitransforms/nonlinear.py Outdated Show resolved Hide resolved
nitransforms/nonlinear.py Outdated Show resolved Hide resolved
@oesteban oesteban changed the title enh: outsource the apply function ENH: Outsource apply() from transform objects May 16, 2024
jmarabotto and others added 13 commits May 16, 2024 08:14
outsourced Apply(); fixed resampled.py (line 101); implemented np.tensordor to _apply_affine() in main.py (line 287). Left to do: fix test_linear RunTime error (line 358)
Outsourced apply, test_linear.py successful
Apply function offsourced. Tests:  139 passed, 163 Skipped, 15 Warnings
nitransforms/nonlinear.py Outdated Show resolved Hide resolved
@oesteban oesteban merged commit 8a18581 into nipy:master May 17, 2024
5 of 6 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.

Offsource .apply() as an independent operation
3 participants