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

Error with include pairs file #189

Open
wpeterman opened this issue May 30, 2019 · 5 comments
Open

Error with include pairs file #189

wpeterman opened this issue May 30, 2019 · 5 comments

Comments

@wpeterman
Copy link

Using v.5.5.3+, I'm getting errors when using the 'include_pairs_file". It appears that the output matrix is 1 column and row smaller when run with the pairs file. Using the example files in the attached ZIP file, row and column 7 are dropped.

test.zip
results_out.zip

@wpeterman
Copy link
Author

Also, it seems that when using the 'include pairs' approach, the run time is >5x longer. Seems like it should be nearly equivalent, or faster. As is, it's faster to run the full data set and then negate/remove unwanted pairs after the full run completes.

@ranjanan
Copy link
Member

@wpeterman Thanks for posting this, you hit an interesting corner case that I didn't consider. Let me explain why this happening:

It turns out that whenever you don't require current or voltage maps to be written to disk and you just need the pairwise resistances, Circuitscape triggers a shortcut to compute all of them, which runs really fast. However, when you want to exclude pairs, Circuitscape is forced to avoid the fast shortcut to check which pairs you want and don't want. That's why when you just want resistances, all pairs is faster than some pairs. I guess the obvious workaround is compute everything anyway (because it's fast) and then mask the pairs you don't want. This won't be faster than the shortcut, but it'll be just as fast. But the include_pairs option saves a bunch of time if you're writing current and voltage maps (on my laptop, include_pairs.ini was twice as fast as all_points.ini). Anyway, I can make this change in a PR and notify you.

Oh, and for your other question: the resistance file is one size smaller because 7 was completely omitted from your include_pairs file.

@ranjanan
Copy link
Member

ranjanan commented Jun 12, 2019

Now that I think about it, I'm more inclined to give you all the resistances without any masking and throw a warning saying that you get the resistances you didn't ask for, for free. Do you have a preference?

@wpeterman
Copy link
Author

Glad I explored the hidden corners of the code for you. Sorry for posting an issue of an atypical scenario (a point excluded). It would seem best/appropriate to have all these excluded values included in the results (as -1), rather than reduce the size of the matrix. Either way, I've currently got a suitable workaround. Thanks for all your time and effort on continuing to improve this!

@ranjanan
Copy link
Member

Sure, I can keep the size of the matrix the same with a PR. Let me check what the old Circuitscape did and maintain the same behaviour.

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

No branches or pull requests

2 participants