search

indica-mcf / Indica

Integrated DiagnostiC Analysis
GNU General Public License v3.0
6 stars 0 forks source link

Marcosertoli/phantom reader #318

Closed marcosertoli closed 5 months ago

marcosertoli commented 5 months ago

Name doesn't fully represent the pull request.

Details of the pull request:

marcosertoli commented 5 months ago

Reasoning is that every time we want to do any Phantom analysis or tests on CI/CD we can load a default Plasma, Equilibrium and a set of Transforms for all diagnostics without having to generate them on the fly. That saves time during tests, as well as development, especially on the Plasma side, and gives us an equilibrium that is more relevant than the current fake_equilibrium that is not fit for purpose.

marcosertoli commented 5 months ago

I had thought of this as being the interface between plasma and profilers. If you decouple them, would you need another class to couple them again?

P.S. ...I keep on having issues with circular imports and would really appreciate if you had some insights on good practice to avoid them from the start!

From: michael-gemmell @.> Sent: Wednesday, April 24, 2024 3:40:40 PM To: indica-mcf/Indica @.> Cc: marcosertoli @.>; Author @.> Subject: Re: [indica-mcf/Indica] Marcosertoli/phantom reader (PR #318)

@michael-gemmell commented on this pull request.

In indica/models/plasma.pyhttps://github.com/indica-mcf/Indica/pull/318#discussion_r1578011513:

@@ -1191,20 +835,121 @@ def call(self): return deepcopy(self.operator())

-def example_run( +class PlasmaProfiles:

I like this - I made something very similar to this myself. I will extend it so that each profile can have different types of profiler_object (important when I implement PCA).

Though I would consider decoupling this from the plasma class. In principle it has no need to have a reference to the plasma, it can just spit out profiles.

— Reply to this email directly, view it on GitHubhttps://github.com/indica-mcf/Indica/pull/318#pullrequestreview-2020078346, or unsubscribehttps://github.com/notifications/unsubscribe-auth/ARZSM6CBXHA6YZDQD2VU43LY6676RAVCNFSM6AAAAABGWZUIVOVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDAMRQGA3TQMZUGY. You are receiving this because you authored the thread.Message ID: @.***>

michael-gemmell commented 5 months ago

Reasoning is that every time we want to do any Phantom analysis or tests on CI/CD we can load a default Plasma, Equilibrium and a set of Transforms for all diagnostics without having to generate them on the fly. That saves time during tests, as well as development, especially on the Plasma side, and gives us an equilibrium that is more relevant than the current fake_equilibrium that is not fit for purpose.

We have to make sure that a new instance of these are created every test so that we are testing properly then.

michael-gemmell commented 5 months ago

I had thought of this as being the interface between plasma and profilers. If you decouple them, would you need another class to couple them again? P.S. ...I keep on having issues with circular imports and would really appreciate if you had some insights on good practice to avoid them from the start!

We would need another class, interface or method to assign the values to the plasma. This is an example of where it is good to separate the logic from the pipeline in my opinion. looking like {profile_name: profiler} -> ProfileManager -> profiles. It is the same pattern as the other context objects (we should discuss this).

I'd need to see where the circular imports are happening to give my opinion, but general advice is circular dependencies are a sign of over-coupling.

marcosertoli commented 5 months ago

I've addressed the comments and fixed another minor detail.

@michael-gemmell, feel free to push the big green button and merge it in if you approve.