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

Make decor mandatory and labels optional. #1978

Merged
merged 7 commits into from
Oct 20, 2022

Conversation

thorstenhater
Copy link
Contributor

@thorstenhater thorstenhater commented Sep 14, 2022

  • Make decor mandatory
  • ...and label arguments to cable cell optional
  • Put the most optional argument (labels) last.

@brenthuisman
Copy link
Contributor

In which cases would a user want a cable cell without decoration?

@thorstenhater
Copy link
Contributor Author

Well, for one it's used quite often in our tests. Second, we had support for it before.

@brenthuisman
Copy link
Contributor

Tests are perhaps not what defaults should be based on. And I can't imagine a simulation of cells without any mechanisms. So unless there is an actual use case, I would like to require it such that people don't forget that cells without e.g. mechanisms are almost certainly not what they want.

@thorstenhater thorstenhater added interface Anything user-facing, including public C++ and Python APIs. AEP Arbor Enhancement Proposal Project: Fippa labels Sep 15, 2022
@thorstenhater
Copy link
Contributor Author

They could have always done that so far, but sure, I see the point. More breaking change then ;)

@thorstenhater thorstenhater removed AEP Arbor Enhancement Proposal Project: Fippa labels Sep 15, 2022
@thorstenhater thorstenhater changed the title Make decor and labels optional. Make decor mandatory and labels optional. Sep 20, 2022
@thorstenhater
Copy link
Contributor Author

Done.

Copy link
Contributor

@boeschf boeschf left a comment

Choose a reason for hiding this comment

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

Looks good - these small changes have quite a substantial ripple effect!
Please merge with master, there are a couple of recipes in the SDE tests that need to be updated.

doc/python/cable_cell.rst Show resolved Hide resolved
python/cells.cpp Outdated Show resolved Hide resolved
@brenthuisman
Copy link
Contributor

@thorstenhater If you could merge master and address @boeschf 's comments, we can merge these breaking changes before v0.8.

@thorstenhater thorstenhater requested a review from boeschf October 13, 2022 10:59
@thorstenhater
Copy link
Contributor Author

Done, I fixed two new warnings in test_sde along with it.

  • sde_recipe& add_probe(probe_tag tag, std::any address) hides a virtual on its base class
  • auto sampler_ = [ncells, nsteps] (std::vector<arb_value_type>& results, unsigned count, doesn't use its ncells capture.

@boeschf
Copy link
Contributor

boeschf commented Oct 13, 2022

there is one more recipe to fix in test_sde, line 826 (only enabled when compiling for GPU)

@thorstenhater
Copy link
Contributor Author

Also, there's the malloc_consolidate: invalid chunk size again.

@thorstenhater
Copy link
Contributor Author

bors try

bors bot added a commit that referenced this pull request Oct 13, 2022
@bors
Copy link

bors bot commented Oct 14, 2022

try

Build failed:

@brenthuisman
Copy link
Contributor

bors try

bors bot added a commit that referenced this pull request Oct 17, 2022
@brenthuisman
Copy link
Contributor

ci/release/build.Dockerfile contains a hardcoded URL to CMake 3.16 (Bors failed on that line, not the tests this time). Since elsewhere there was a bump, we could do so here too.

@bors
Copy link

bors bot commented Oct 17, 2022

try

Build failed:

@thorstenhater
Copy link
Contributor Author

That file hasn't changed in a while and I have this:

RUN wget -q "https://github.com/Kitware/CMake/releases/download/v3.18.6/cmake-3.18.6-Linux-x86_64.tar.gz" -O cmake.tar.gz && \
    echo "87136646867ed65e935d6bacd44d52a740c448ad0806f6897d8c3d47ce438c8b cmake.tar.gz" | sha256sum --check --quiet && \
    tar --strip-components=1 -xzf cmake.tar.gz -C /usr/local && \
    rm -rf cmake.tar.gz

@thorstenhater thorstenhater merged commit cef0d1d into arbor-sim:master Oct 20, 2022
@thorstenhater thorstenhater deleted the ux/simpler-cable-cell branch October 20, 2022 13:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
interface Anything user-facing, including public C++ and Python APIs.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants