Skip to content

Renaming and Back Compatibility #457

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.

Sign up for GitHub

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

Jump to bottom
Merged
FilippoOlivo merged 7 commits into from
Feb 24, 2025
Merged

Renaming and Back Compatibility #457

FilippoOlivo merged 7 commits into from
Feb 24, 2025

Conversation

@dario-coscia
Copy link
Collaborator

@dario-coscia dario-coscia commented Feb 19, 2025

In this PR I rename some modules using a singular convention #445 + add back compatibility for 0.1

@dario-coscia dario-coscia added pr-to-review Label for PR that are ready to been reviewed v0.2 labels Feb 19, 2025
@dario-coscia dario-coscia requested a review from ndem0 February 19, 2025 15:32
@dario-coscia dario-coscia self-assigned this Feb 19, 2025
@dario-coscia dario-coscia mentioned this pull request Feb 19, 2025
Merged
FilippoOlivo
FilippoOlivo reviewed Feb 19, 2025
View reviewed changes
pina/model/block/avno_layer.py
Copy link
Member

@FilippoOlivo FilippoOlivo Feb 19, 2025

Choose a reason for hiding this comment

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

Why not avno_block.py ?

Copy link
Collaborator

@GiovanniCanali GiovanniCanali Feb 20, 2025

Choose a reason for hiding this comment

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

I would go for average_neural_operator_block.py

pina/model/block/fourier.py Outdated
from ...utils import check_consistency

from pina.model.layers import (
from pina.model.block import (
Copy link
Member

@FilippoOlivo FilippoOlivo Feb 19, 2025

Choose a reason for hiding this comment

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

is there a way to avoid from pina? Does from . import ... work?

Copy link
Member

@ndem0 ndem0 Feb 20, 2025

Choose a reason for hiding this comment

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

Yes, you should always prefer the relative import

pina/model/block/lowrank_layer.py
Copy link
Member

@FilippoOlivo FilippoOlivo Feb 19, 2025

Choose a reason for hiding this comment

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

isn't better lowrank_block ?

pina/model/block/rbf_layer.py
Copy link
Member

@FilippoOlivo FilippoOlivo Feb 19, 2025

Choose a reason for hiding this comment

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

rbf_block ?

pina/solver/rom.py Outdated
import torch

from pina.solvers import SupervisedSolver
from pina.solver import SupervisedSolver
Copy link
Member

@FilippoOlivo FilippoOlivo Feb 19, 2025

Choose a reason for hiding this comment

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

What do you think about from . import SupervisedSolver?

Copy link
Member

@ndem0 ndem0 Feb 20, 2025

Choose a reason for hiding this comment

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

Yes, you should always prefer the relative import

GiovanniCanali
GiovanniCanali reviewed Feb 20, 2025
View reviewed changes
Copy link
Collaborator

@GiovanniCanali GiovanniCanali left a comment

Choose a reason for hiding this comment

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

Sorry for the spam. All suggestions refer to renaming files, following the guidelines in the Discussions. In addition to the commented files, we might rename file in test_model directory as well. What do you think?

pina/model/block/messa_passing.py Outdated
Copy link
Collaborator

@GiovanniCanali GiovanniCanali Feb 20, 2025

Choose a reason for hiding this comment

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

I would suggest to change the name from messa_passing.py to message_passing.py

Copy link
Collaborator Author

@dario-coscia dario-coscia Feb 20, 2025

Choose a reason for hiding this comment

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

Ok, I removed the file cuz it was empty

pina/adaptive_function/adaptive_func.py
Copy link
Collaborator

@GiovanniCanali GiovanniCanali Feb 20, 2025

Choose a reason for hiding this comment

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

I would suggest to change the name from adaptive_func.py to adaptive_function.py

pina/adaptive_function/adaptive_func_interface.py
Copy link
Collaborator

@GiovanniCanali GiovanniCanali Feb 20, 2025

Choose a reason for hiding this comment

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

I would suggest to change the name from adaptive_func_interface.py to adaptive_function_interface.py

pina/callback/adaptive_refinment_callback.py
Copy link
Collaborator

@GiovanniCanali GiovanniCanali Feb 20, 2025

Choose a reason for hiding this comment

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

Typo: adaptive_refinement_callback

pina/model/avno.py
Copy link
Collaborator

@GiovanniCanali GiovanniCanali Feb 20, 2025

Choose a reason for hiding this comment

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

I would suggest to rename from avno.py to average_neural_operator.py

tests/test_blocks/test_spectral_conv.py
Copy link
Collaborator

@GiovanniCanali GiovanniCanali Feb 20, 2025

Choose a reason for hiding this comment

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

I would rename it as test_spectral_convolution.py

tests/test_callback/test_adaptive_refinment_callback.py
Copy link
Collaborator

@GiovanniCanali GiovanniCanali Feb 20, 2025

Choose a reason for hiding this comment

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

Typo: test_adaptive_refinement_callback.py

tests/test_data/test_datamodule.py
Copy link
Collaborator

@GiovanniCanali GiovanniCanali Feb 20, 2025

Choose a reason for hiding this comment

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

I would suggest: test_data_module.py

tests/test_equations/test_systemequation.py
Copy link
Collaborator

@GiovanniCanali GiovanniCanali Feb 20, 2025

Choose a reason for hiding this comment

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

I would suggest test_system_equation.py

tests/test_solver/test_rom_solver.py
Copy link
Collaborator

@GiovanniCanali GiovanniCanali Feb 20, 2025

Choose a reason for hiding this comment

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

What about: test_reduced_order_model.py?

@dario-coscia
Copy link
Collaborator Author

dario-coscia commented Feb 20, 2025

Thanks @gc031298 and @FilippoOlivo for the suggestions, I will try to fix them asap!

dario-coscia and others added 4 commits February 21, 2025 09:00
@dario-coscia @FilippoOlivo
Modify pina.__init__.py and rm useless .py files
9bb3c71 
* rm meta.py, plotter.py, writer.py
* modify __init__ file
* modify tests due to __init__ import
@dario-coscia @FilippoOlivo
Renaming
b84839a 
* solvers -> solver
* adaptive_functions -> adaptive_function
* callbacks -> callback
* operators -> operator
* pinns -> physics_informed_solver
* layers -> block
@dario-coscia @FilippoOlivo
Back compatibility 0.1
d62e75b
@FilippoOlivo
Rename files
3d96141
@FilippoOlivo FilippoOlivo requested review from FilippoOlivo and removed request for FilippoOlivo February 21, 2025 08:03
@GiovanniCanali
Copy link
Collaborator

GiovanniCanali commented Feb 21, 2025

Ready to merge on my side!

@FilippoOlivo
Copy link
Member

FilippoOlivo commented Feb 21, 2025

Ready to merge on my side!

Also for me!

@dario-coscia dario-coscia force-pushed the simplify_package branch 2 times, most recently from b003319 to 37c3caf Compare February 24, 2025 10:20
@dario-coscia
Update installations and workflows
eebb1c2 
* Update pyproject.toml
* Create a Workflow file to test formatting in PR
@dario-coscia
Formatting
9cd9412 
* Adding black as dev dependency
* Formatting pina code
* Formatting tests
@FilippoOlivo FilippoOlivo merged commit 12b7787 into 0.2 Feb 24, 2025
16 checks passed
@dario-coscia dario-coscia deleted the simplify_package branch February 26, 2025 18:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@GiovanniCanali GiovanniCanali GiovanniCanali left review comments

@FilippoOlivo FilippoOlivo FilippoOlivo left review comments

@ndem0 ndem0 ndem0 approved these changes

Assignees

@dario-coscia dario-coscia

Labels

pr-to-review Label for PR that are ready to been reviewed

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@dario-coscia @GiovanniCanali @FilippoOlivo @ndem0