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

Simulation Object Restructure (First Phase) #2212

Closed
wants to merge 6 commits into from

Conversation

Soumya624
Copy link

📝 Description

Type: 🚀 feature

Created a new class ConvergencePlot under the file name tardis/simulation/convergence_plot.py and move damped_converge and _get_convergence_status methods in that. Changes are there in tardis/simulation/base.py and tardis/simulation/convergence_plot.py

📌 Resources

Examples, notebooks, and links to useful references.

🚦 Testing

How did you test these changes?

  • Tested Locally

☑️ Checklist

  • I requested two reviewers for this pull request
  • I updated the documentation according to my changes
  • I built the documentation by applying the build_docs label

@Soumya624
Copy link
Author

@Rodot- Please have a look at this PR now. Tried to maintain a fixed commit history. Three files have been modified, including the mailmap



class ConvergencePlot:
@staticmethod
Copy link
Member

Choose a reason for hiding this comment

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

if it's a static method, you don't need self as an argument.

Copy link
Author

@Soumya624 Soumya624 Feb 20, 2023

Choose a reason for hiding this comment

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

@sonachitchyan Tried to remove it. But, few tests were breaking

Copy link
Contributor

Choose a reason for hiding this comment

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

Did you remove the self from the calls in the simulation as well? What was the error?

Copy link
Author

Choose a reason for hiding this comment

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

@Rodot- I think now it's resolved. Please have a look!

@codecov
Copy link

codecov bot commented Feb 20, 2023

Codecov Report

Merging #2212 (94d2fdd) into master (f08c8f5) will increase coverage by 0.01%.
The diff coverage is 100.00%.

❗ Current head 94d2fdd differs from pull request most recent head e8228dd. Consider uploading reports for the commit e8228dd to get more accurate results

@@            Coverage Diff             @@
##           master    #2212      +/-   ##
==========================================
+ Coverage   71.81%   71.82%   +0.01%     
==========================================
  Files         133      134       +1     
  Lines       12355    12360       +5     
==========================================
+ Hits         8873     8878       +5     
  Misses       3482     3482              
Impacted Files Coverage Δ
tardis/simulation/base.py 82.52% <100.00%> (-1.48%) ⬇️
tardis/simulation/convergence_plot.py 100.00% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

tardis/simulation/base.py Outdated Show resolved Hide resolved
def damped_converge(value, estimated_value, damping_factor):
return value + damping_factor * (estimated_value - value)

def _get_convergence_status(
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this meant to be a static method?

Copy link
Author

Choose a reason for hiding this comment

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

@Rodot-
I thought, since damped_converge does not depend on any instance variables or attributes of the ConvergencePlot class, we can make it a static method instead of an instance method. Also this can allow it to be used more flexibly outside the context of ConvergencePlot class (If needed).

Please let me know if I need to make any changes!

logger = logging.getLogger(__name__)


class ConvergencePlot:
Copy link
Contributor

Choose a reason for hiding this comment

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

This class seems to be able to do everything with static methods and there's no init. Does this actually need to be a class or could the whole thing just be a module? If it is a class, what attributes does it have and keep track of?

Copy link
Author

Choose a reason for hiding this comment

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

@Rodot-
Yeah we can refactor this class as a module with standalone functions. But, even though the current implementation of ConvergencePlot does not have any instance variables, it may be useful to keep it as a class if there is a possibility that instance variables may be added in the future to track and store the convergence data across multiple function calls.

Please let me know if there's any changes required!

Copy link
Contributor

@Rodot- Rodot- left a comment

Choose a reason for hiding this comment

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

I think you should include the plotting part of advance_state in the ConvergencePlot class as well. Ideally, the class should entirely handle all of the convergence information and calculations and simply give back the results to be incorporated into the simulation

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants