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

Testing n_columns=0 #299

Merged
merged 1 commit into from
Apr 29, 2021

Conversation

HenrikMettler
Copy link
Contributor

So, weirdly n_columns = 0 works in the minimal example and in a small test in population it gives the expected sympy expression. However a lower level test fails when calling directly CartesianGraph(genome).to_sympy(). Any ideas why?
Also imo we should rather deprecate the n_columns=0 or n_rows=0 functionality, since they don't make much sense anyway.
Closes #283

@HenrikMettler HenrikMettler marked this pull request as draft April 13, 2021 16:55
@jakobj jakobj modified the milestones: 0.4.0, 0.3.0 Apr 19, 2021
Copy link
Member

@jakobj jakobj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

great, thanks for working on this! 🚀

i've added a few inline comments that should be addressed.

regarding your question: the low-level error was generated because the genome was not initialized. in also, i think we should not explicitly forbid n_columns=0 if our library does the right thing, why not?

test/test_genome.py Outdated Show resolved Hide resolved
test/test_genome.py Outdated Show resolved Hide resolved
test/test_population.py Outdated Show resolved Hide resolved
test/test_population.py Outdated Show resolved Hide resolved
test/test_population.py Outdated Show resolved Hide resolved
@HenrikMettler HenrikMettler marked this pull request as ready for review April 23, 2021 12:25
@HenrikMettler
Copy link
Contributor Author

Ok, i am a bit lost. Somehow the test doesn't pass on the python 3.7 testsuite, however I can't reproduce the error locally. Any ideas why?

@mschmidt87
Copy link
Member

I also tried to reproduce the error locally without success. I also tried to google the error and found this link which might (or might not) be helpful: https://stackoverflow.com/questions/66640404/basic-loop-with-sympy-gives-me-this-error-message-symbol-object-is-not-subsc
Another, a bit brute-force option, is to do:

  • remove python 3.7 temporarily to see whether 3.6 and 3.8 throw the same error in the CI
  • add some debug output to the test and rerun the CI.
    Also, I did create a new conda env with python 3.7 and installed the library, so I basically followed the steps in the CI, but I didn't confirm that the configuration is really exactly identical with the CI environment. So, you could/should also do that with your local environment.

@jakobj
Copy link
Member

jakobj commented Apr 27, 2021

have you tried rebasing on the most recent master? one of the recent PRs has removed the necessity to do sympy_expr[0] if only one expression is encoded in the genome, as it is automatically unpacked. i think this might be where the error comes from.

@HenrikMettler
Copy link
Contributor Author

ah yes, that was it! my bad, thanks for suggesting

Copy link
Member

@jakobj jakobj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looking good! 👍

and merging

@jakobj jakobj merged commit 9af8b31 into Happy-Algorithms-League:master Apr 29, 2021
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.

Make sure n_columns = 0 works properly
3 participants