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

modify add_models() #28

Open
wants to merge 1 commit into
base: refactor
Choose a base branch
from
Open

Conversation

nonsk131
Copy link

modify add_models() so that it assigns _0 to the brightest star in the system

@timothydmorton
Copy link
Owner

Great, thanks-- I'm looking over this and have a few suggestions. I'll get back to you soon (have to go to a meeting now but will be back and then will go through this and send comments).

@timothydmorton
Copy link
Owner

Actually, I'm going to be delayed a bit more, but I will get back to this hopefully later today.

@timothydmorton
Copy link
Owner

OK, a few initial comments:

  • the get_mag_leaf function depends on a particular naming convention of each observed leaf, one that is not really enforced anywhere, so this is not a reliable way to get the magnitude from an ObsNode. Instead, you should take advantage of the fact that you can access the observed magnitude via n.value[0], if n is the node in question.
  • This is somewhat of a corner case, but it shows up in K00177, the example case I've been using to check things---the magnitude comparison can only be between the same instruments... that is, it makes no sense to compare a magnitude from one survey to that of another. For example, in K00177, my default model definition assigns model nodes to four stars observed by Kraus, though only two of them are observed by Robo-AO. So in this case, define_models needs to know what instrument is shared by all the chosen leaves, and compare those observed magnitudes.
  • We need to be able to handle the case that assigns more than one model node to a given observed star. It's not clear that this current implementation will allow this.

Anyway, this is a good start on a tough problem! I'll be thinking about this more; stay tuned! Feel free to use this implementation if it works for your current synthetic simulations, but our goal will be something a bit more robust.

@timothydmorton
Copy link
Owner

timothydmorton commented Jul 21, 2016

Another solution that might be even more robust is just to have the _get_leaves and select_leaves functions to by default return leaves in some sort of ordered fashion, since this is currently what dictates the labels of the model nodes...

Or to just raise a warning if things end up out of order, and then have a separate method do the rearranging if desired?... I'm just brainstorming here, the best solution isn't obvious to me!

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

Successfully merging this pull request may close these issues.

2 participants