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

add gitignore; extend gather indexes to signle coord #42

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

Conversation

YingtianDt
Copy link

@YingtianDt YingtianDt commented Sep 13, 2023

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
Copy link
Author

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
Copy link
Member

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
Copy link
Author

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
Copy link
Member

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

@YingtianDt
Copy link
Author

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

@deirdre-k
Copy link

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
Copy link
Member

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

@YingtianDt
Copy link
Author

YingtianDt commented Jun 5, 2024

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
Copy link

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
Copy link
Author

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
Copy link
Author

Hello, I am testing the new gather_index (brain-score/vision#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
Copy link
Author

YingtianDt commented Jul 15, 2024

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

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.

3 participants