Unifying the code for finding grid boundaries and checking validity in case of ignored data. #271
Conversation
|
Update (Apr 11): Code does not affect logic - just a more elegant solution to finding grid boundaries with fewer lines of code. @kkovlakas to test this with a grid to make sure boundaries are unaffected. |
There was a problem hiding this comment.
This is a nice simplification of the code.
@kkovlakas I was just wondering if a function definition inside a function is a good idea, especially since this is quite a general function. It might be better to define it as another private function of the class. That will also be easier for testing.
Otherwise it looks fine!
|
@kkovlakas, have you had a chance to test this PR yet? |
I think this function is not going to be used in other methods of the class. So I would keep it hidden to be really "private". |
Not yet. I was having some issues with Quest, but I am planning to continue again this week. |
|
Update (Aug 22): This is a very minor update that we should be able to accept with not too much conversation in the future. One check: we want to make sure that in the CO-HMS_RLO grid, we aren't accidentally setting the boundary to models that never overfilled their Roche lobes. Those simulated points are still stored in the grids for bookkeeping, even if they never interacted. |
|
@kkovlakas asked me to check the grid boundaries between the dev and this PR: HMS-HMSPR271 dev CO_HMS_RLOPR dev CO_HeMS_RLOPR dev CO_HeMSPR dev |
|
The difference in the m1_max values in the RLO grids between dev and this PR are indeed from runs, where the final values of the mass are |
| """Infer the grid boundaries (min/max of masses and orbital period).""" | ||
| def initial_values_min_max(parameter_name): | ||
| # get the request column from the initial values | ||
| arr = self._psyTrackInterp.grid.initial_values[parameter_name] |
There was a problem hiding this comment.
I'd suggest to give it a better name then arr, especially when having final in mind.
| return np.min(arr), np.max(arr) | ||
|
|
||
| self.m1_min, self.m1_max = initial_values_min_max('star_1_mass') | ||
| self.m2_min, self.m2_max = initial_values_min_max('star_2_mass') |
There was a problem hiding this comment.
Beside m2 should we always calculate a q_min and q_max? At the moment this is still hard coded as 0.05 and 1.0 for the HMS-HMS grid.
| state_2 == 'H-rich_Core_H_burning' and | ||
| event == 'ZAMS' and | ||
| self.m1_min <= m1 <= self.m1_max and | ||
| np.max([self.q_min, 0.5/m1]) <= mass_ratio <= self.q_max and |
There was a problem hiding this comment.
Shouldn't we better use np.max([self.q_min, self.m2_min/m1])? The same applies at other places, too.
ignoreas a way to determined ignored/failed systems?