Skip to content
This repository has been archived by the owner on Sep 1, 2023. It is now read-only.

Fix failed test SimpleTwoRegionNetworkIntrospection on 32 bit Linux #1438

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

paulscode
Copy link
Contributor

This solves issue #1437. New to swig, so may not be the best solution. Should at least highlight the source of the failure. Please share any ideas for solving the problem differently.

@rhyolight
Copy link
Member

@lscheinkman do you mind reviewing this when you have time?

@rhyolight
Copy link
Member

@paulscode Have you never signed the contributor license?

@paulscode
Copy link
Contributor Author

I signed it yesterday. Do I need to do anything else to make the validator pick it up?

@rhyolight
Copy link
Member

rhyolight commented Sep 12, 2018

It should pick it up now. If not, ignore the validation failure I have confirmed you've signed it.

@rhyolight
Copy link
Member

Both #1438 & #1440 introduce windows build errors, can you check them out?

@rhyolight
Copy link
Member

Nevermind Paul, those windows build errors were there already. Luiz is planning on working on it but not sure exactly when.

@paulscode
Copy link
Contributor Author

Resolved the standards-related error reported by bamboo

@rhyolight
Copy link
Member

@paulscode can you merge in master as soon as #1442 is merged? Then CI will run again with his changes.

Copy link
Contributor

@lscheinkman lscheinkman left a comment

Choose a reason for hiding this comment

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

@paulscode Could you add a test for the new Dimensions(std::vector<unsigned long> v) constructor?
See https://github.com/numenta/nupic.core/blob/master/src/test/unit/ntypes/DimensionsTest.cpp#L176

@paulscode
Copy link
Contributor Author

paulscode commented Oct 1, 2018

@lscheinkman, doesn't the testSimpleTwoRegionNetworkIntrospection test cover this constructor? Or you looking for something specific for only testing the constructor by itself?

@lscheinkman
Copy link
Contributor

@paulscode You are correct, testSimpleTwoRegionNetworkIntrospection covers this constructor when the python code is running on linux32. I was referring to a pure C++ test specific for the constructor on linux32 platform.

@rhyolight
Copy link
Member

@paulscode I think this is waiting for one more C++ test.

@paulscode
Copy link
Contributor Author

Oh, right. Thanks for the reminder.

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

Successfully merging this pull request may close these issues.

None yet

3 participants