Skip to content

Implement access to device and dtype#65

Merged
SCiarella merged 18 commits into
mainfrom
gpu
Jan 8, 2026
Merged

Implement access to device and dtype#65
SCiarella merged 18 commits into
mainfrom
gpu

Conversation

@SCiarella
Copy link
Copy Markdown
Collaborator

@SCiarella SCiarella commented Dec 16, 2025

Closes issue #23

This PR exposes device and dtype such that they can be controlled and set externally by the user. This is particularly relevant for running diffwofost on GPU by setting device="cuda". There are example of this in the notebooks and in the documentation.

The tests have been update by adding a new @pytest.fixture that checks if a GPU is available, and if so, it re-runs the most important tests on the GPU a second time. Notice that GitHub actions do not provide GPU access (for free), so I have been using my local hardware to test the code.


Notice that without finalizing the Engine, the GPU device could be slower than the CPU, due to temporary fixes like the use of deepcopy in the TestEngine.

@SCiarella SCiarella marked this pull request as draft December 16, 2025 11:59
@SCiarella SCiarella marked this pull request as ready for review December 19, 2025 09:37
Copy link
Copy Markdown
Collaborator

@fnattino fnattino left a comment

Choose a reason for hiding this comment

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

Nice work @SCiarella! I have left a few comments, note that not all comments are suggestions for changes - some are just notes for future work/discussion. Anyway, feel free to address only what you think makes sense!

Comment thread docs/notebooks/optimization.ipynb
Comment thread pyproject.toml
Comment on lines +123 to +126
# Default values that can be overridden before instantiation
device = torch.device("cuda" if torch.cuda.is_available() else "cpu")
dtype = torch.float64

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I suspect the default device and dtype will be the same everywhere? So maybe we could define default values of DTYPE and DEVICE at a global level, e.g. in a defaults.py module, instead of having it defined for each module/model? What do yout think?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Good point! Yes, they have to be the same everywhere, so it makes sense to follow a more centralized approach that is accessible for the user.

In the next iteration of this PR, I have added to config.py a new class called ComputeConfig that globally controls them. In its docstring, I have reported a small example on how to use that, but we should probably link that somewhere else if we want to use this class to control the global behavior.

Comment thread tests/physical_models/crop/test_leaf_dynamics.py Outdated
Comment thread tests/physical_models/crop/test_leaf_dynamics.py Outdated
Comment thread src/diffwofost/physical_models/crop/leaf_dynamics.py
Comment thread src/diffwofost/physical_models/crop/root_dynamics.py Outdated
Comment thread src/diffwofost/physical_models/crop/phenology.py Outdated
Comment thread src/diffwofost/physical_models/crop/phenology.py Outdated
Comment thread src/diffwofost/physical_models/crop/phenology.py
@SCiarella
Copy link
Copy Markdown
Collaborator Author

Thanks @fnattino for the careful review!

I have implemented most of the suggestions that you proposed. In particular, I introduced a new class in config.py called ComputeConfig that is used to set globally dtype and device. Using this global control, I was able to remove most of the redundant type checks that you also pointed out. @SarahAlidoost what do you think of this solution?

There is a point that remains open regarding the kiosk. Currently, I have implemented some type/device conversion for the variables that come from the kiosk. Later, we should ensure that the engine enforces global types and devices, such that the kiosk does not need to be converted every time.

@SCiarella SCiarella requested a review from fnattino January 7, 2026 15:38
@sonarqubecloud
Copy link
Copy Markdown

sonarqubecloud Bot commented Jan 8, 2026

Copy link
Copy Markdown
Collaborator

@fnattino fnattino left a comment

Choose a reason for hiding this comment

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

Thanks a lot for having a look at all the comments @SCiarella - looks good to me!

@SarahAlidoost
Copy link
Copy Markdown
Collaborator

Thanks @fnattino for the careful review!

I have implemented most of the suggestions that you proposed. In particular, I introduced a new class in config.py called ComputeConfig that is used to set globally dtype and device. Using this global control, I was able to remove most of the redundant type checks that you also pointed out. @SarahAlidoost what do you think of this solution?

I like ComputeConfig implementation. Clean and clear. 👍

There is a point that remains open regarding the kiosk. Currently, I have implemented some type/device conversion for the variables that come from the kiosk. Later, we should ensure that the engine enforces global types and devices, such that the kiosk does not need to be converted every time.

yes lets make an issue for this to not forget it.

Copy link
Copy Markdown
Collaborator

@SarahAlidoost SarahAlidoost left a comment

Choose a reason for hiding this comment

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

@SCiarella thanks for the implementation 🥇 All good to me. Nice that you could test the gpu part on your system. Later in #58, we will find alternatives.

@SCiarella SCiarella merged commit c292d3b into main Jan 8, 2026
11 checks passed
@SCiarella SCiarella deleted the gpu branch January 8, 2026 15:39
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