brain-score / brainio

Data management for quantitative comparison of brains and brain-inspired systems
MIT License
5 stars 12 forks source link

add gitignore; extend gather indexes to signle coord #42

Open YingtianDt opened 1 year ago

YingtianDt commented 1 year ago

This is to solve the following problem:

# A small trick to preserve single-coord multi-index
# Problem: you want to have a multi-index "x" with a single coord "x-coord"

# Try using set_index
print(xr.DataArray([1], dims=["x"], coords={"x-coord1": ("x", [0]), "x-coord2": ("x", [0])}).set_index(x=["x-coord1", "x-coord2"]), end="\n\n")  # will work for multiple coords
print(xr.DataArray([1], dims=["x"], coords={"x-coord": ("x", [0])}).set_index(x=["x-coord"]), end="\n\n")                                        # will NOT work for single coord

# Solution: use stack
print(xr.DataArray([1], dims=["x-coord"], coords={"x-coord": ("x-coord", [0])}).stack(x=["x-coord"]))  

With the original gather_index, the multi-index with a single coord will lose the coord, by using the set_index. With the current edit, the stacking trick is performed to preserve that single coord.

YingtianDt commented 4 months ago

Either people should consider this PR, or should consider removing gather_index. This bug has cost me tons of time when I tried to do anything with DataAssembly. And from the other users I've talked to, they are also struggling with it.

So please consider this.

mschrimpf commented 4 months ago

unit tests seem to be failing -- can you please check? we should run the brain-score vision and language unit tests as well to make sure this does not break anything.

YingtianDt commented 3 months ago

Hi, the tests are failing mostly due to the "one coordinate one dimension" conditions in those tests, and there are many of them, maybe even across brainio and brainscore_vision. I can try to start fixing all of them, but I wonder if I can get helps on at least a detailed review of the tests that I am gonna change.

mschrimpf commented 3 months ago

The Quest engineering team is pretty much booked for the next few months, but you could ask in the slack

YingtianDt commented 3 months ago

I made brainio tests passed. Could you give suggestions on how I can test with brainscore_vision integratively?

deirdre-k commented 3 months ago

Hi @YingtianDt. Thanks for putting so much work into this!

I've opened draft PRs #896 in vision and #238 to trigger the unit tests to run with the changes from your fork. This should trigger all non-plugin unit tests for each repository, plus tests for one of the benchmarks in each. @mschrimpf if there are any specific plugins you would recommend testing I'd be happy to adjust those.

mschrimpf commented 3 months ago

sounds good @deirdre-k, I don't think we need to test anything beyond the standard tests

YingtianDt commented 3 months ago

Hi @deirdre-k , thank you so much for your help! I think at this point the PR won't pass because both vision/language repo need edits to make the new gather_index compatible -- essentially, they don't have to deal with the "single coordinate single dimension" case anymore.

I will change the two repos individually and make PR to them. And I've checked how you link my version of the brainio into the PR tests (by changing the requirement source). I will also do this myself when I do the PRs.

With these said, I noticed that some of the Travis tests end up with

× Getting requirements to build wheel did not run successfully.
  │ exit code: 1
  ╰─> [37 lines of output]
      Package hdf5 was not found in the pkg-config search path.
      Perhaps you should add the directory containing `hdf5.pc'
      to the PKG_CONFIG_PATH environment variable
      No package 'hdf5' found
      reading from setup.cfg...

May I ask if this is normal? And do I have to make all of the Travis tests (sometimes there are multiple "Job"s with public/private as suffix) passed?


Please still keep the two PRs from your side, so that I can see some related errors. The edits to the two repos will take some time. Thank you again for your effort!

deirdre-k commented 3 months ago

No problem! Absolutely, I'll leave those open for you. I would focus on getting the Jenkins tests to pass. When you're ready to open your finalized PR, we can take a look at any failing Travis tests to double check for any missed issues, but we'll prioritize Jenkins.

Regarding the test failure, it looks like the netcdf4 library version is pinned under 1.6.5 in the main brainio repo for python 3.7 compatibility reasons. I think your fork may have fallen behind the main repo, you may want to pull in the latest changes in!

YingtianDt commented 3 months ago

Hi, I test the changes locally and surprisingly the new gather_index seems to be compatible with the old codes and tests in brainscore_vision. So I think we can just give it a go with #896 and #238. But it seems the tests in vision have been pending for a week. Maybe we can try re-triggering it?

YingtianDt commented 3 months ago

Hello, I am testing the new gather_index (https://github.com/brain-score/vision/pull/963) and it seems to be magically compatible with the old codes, so there is no need to change other codes except some tests as I did in this PR. In the testing PR 963, there are some errors but they seem to have nothing to do with gather_index.

Let me know what you think and if we can merge this PR. Thanks!

YingtianDt commented 2 months ago

Hi @deirdre-k , based on the new results here, do you think we can merge this PR?