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

Remove magic from _ShockFactory #15

Open
astrojuanlu opened this issue Dec 9, 2013 · 6 comments
Open

Remove magic from _ShockFactory #15

astrojuanlu opened this issue Dec 9, 2013 · 6 comments
Milestone

Comments

@astrojuanlu
Copy link
Member

Right now the code of _ShockFactory may be smart but it's full of black magic. Very difficult to follow, would be better if more explicit.

@Gillu13
Copy link
Contributor

Gillu13 commented Jun 9, 2014

hello Juan,

Tell me if I'm wrong but I understand this piece of code like this:

One can define a Shock object by using various combinations of entry arguments: ['X', 'beta', 'gamma'] or ['X', 'theta', 'weak', 'gamma'], X being either M_1, M_2 or p1_p2 (if i'm not wrong again, M_1 is the only implemented one so far).

The _ShockClass's constructor however takes only a certain combination of arguments: ['M_1', 'beta', 'gamma']. Thus, one needs intermediary private functions (_ShockFactory() and _from_deflection_angle()) in order to convert input arguments into the accepted combination and eventually construct the shock wave object.

I've silly and naive questions: why not simply including in _ShockClass.__init__() the task of dealing with **kwargs? Or, if an explicit argmuments management is needed, why not creating a basic Shock class with common properties and several subclasses that inherit properties from Shock but have different constructors (depending on known parameters)? Like this it would be possible to have a NormalShock class, remaining consistent with the nozzles module for instance, which uses the NormalShock class .

@astrojuanlu
Copy link
Member Author

Excuse me Gilles, you caught me in exams time and I have some
constraints but I saw this and I'll give you an answer as soon as I can
:) In the meanwhile, the two offending commits are 49d1379 and 6a8a7ad.

@Gillu13
Copy link
Contributor

Gillu13 commented Jun 11, 2014

hello Juan,

Don't worry, there is no rush. Good luck for your exams! In the meanwhile, I'll have a look at those commits.

@Gillu13
Copy link
Contributor

Gillu13 commented Mar 26, 2015

Hello Juan,

I am wondering if you are still working on this code? Does it worth commiting improvements?

Regards,

Gilles

@astrojuanlu
Copy link
Member Author

Hello @Gillu13, thanks for your interest. The truth is that it's been a long time since I last worked on this, but I am not sure if I will ever do again or not. I am in the process of teaching Python to some friends (cc @AeroPython/ISA) and we will have to decide what to do with this code.

My suggestion is that, if you are interested in developing this, you commit your changes in your fork as you desire. In some weeks or months we can decide the future of the project.

Best regards and thanks again 😄

@Gillu13
Copy link
Contributor

Gillu13 commented Mar 27, 2015

Hello Juan,

thanks for your answer. OK, let's do this.

Best regards.

Gilles

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

No branches or pull requests

2 participants