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

Drop na results from Monte-Carlo simulation results to enable Spearman ranking #38

Closed
wants to merge 542 commits into from

Conversation

yalinli2
Copy link
Member

This is a very minor change but I think it's helpful if you run 1,000 simulation and only 3 or so fails. I also edited the gitignore file to resolve a conflict. All tests have been passed.

Screen Shot 2020-09-18 at 11 20 52 AM

@yoelcortes
Copy link
Member

yoelcortes commented Sep 19, 2020

Thanks for the pull request. Although dropping NA values may seem convenient, there are a couple of downsides to this:

  1. Dropna doesn't care if 1 or 90% of the values are NA, so these possible errors would pass silently. In fact, I believe any NA should not be tolerated.
  2. Adds overhead to the function that is not necessary for most cases.
  3. NA values may want to be replaced with other values by the user. For example, evaluating a metric that is either True or False, and False if the biorefinery could not be solved for.
  4. The dropna() method would apply to every row with NA values. If Spearman's correlation is needed for columns that doesn't have NA values (while other columns do), we would be loosing usable data.

The user is free to change values in the table, and even replace the table attribute with a DataFrame of their own. If you'd like to drop NA values, its as easy as:

# Where model is a biosteam.evaluation.Model object
model.table = model.table.dropna()

Thanks!

@yoelcortes yoelcortes closed this Sep 19, 2020
@yalinli2
Copy link
Member Author

OK sounds good, thanks!

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.

None yet

2 participants