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

To work around #6327 #6333

Closed
wants to merge 4 commits into from
Closed

To work around #6327 #6333

wants to merge 4 commits into from

Conversation

arafune
Copy link
Contributor

@arafune arafune commented Jul 19, 2024

While this PR is not still sufficient to work around #6327, such revision must be required at least.

The essential point of the change is the following part.

  •        kdim_len = len(kdim_param.default) if kdims is None else len(kdims)
    
  •        if kdims is None:
    
  •            kdims = list(data.dims)
    
  •        kdim_len = len(kdims)
    

While reading the code, I found the following part difficult to understand:

if not isinstance(data, (xr.Dataset, xr.DataArray)):

To improve readability, I changed it to a more straightforward approach. I hope you will accept this change as well.

@arafune
Copy link
Contributor Author

arafune commented Jul 23, 2024

Do you need further information? Why is there no response at all?

@ahuang11
Copy link
Collaborator

ahuang11 commented Jul 24, 2024

Thanks for creating this PR.

While this PR is not still sufficient to work around #6327, such revision must be required at least.

Can you elaborate a bit on this? What else is needed?

Taking a glance at the code changes, can you explain what the changes aim to fix?

@arafune
Copy link
Contributor Author

arafune commented Jul 24, 2024

Thanks for creating this PR.

While this PR is not still sufficient to work around #6327, such revision must be required at least.

Can you elaborate a bit on this? What else is needed?

Sorry for the confusion. I have checked against a simple Xarray object and as far as I can tell the problem is solved, just not 100% sure.

Taking a glance at the code changes, can you explain what the changes aim to fix?

I think yes. The change is really tiny.
In the current version, the assignment to the X- and Y-axis is based on the coords object, but coords is not a sequence, which means that the order is not constant.

@arafune
Copy link
Contributor Author

arafune commented Jul 30, 2024

What is the further information to accept this PR?

@hoxbro
Copy link
Member

hoxbro commented Jul 30, 2024

Currently, tests are failing because of your changes. Some seem trivial fixes (moving order around), while others do not.

Making it so the tests pass will be the next step.

@arafune
Copy link
Contributor Author

arafune commented Jul 30, 2024

@hoxbro
Thank you for your comment.

Unfortunately, different from the result of GitHub Actions, most of the tests have passed on my local system (Mac).

SS2024-07-31 8 25 31

Only the fault is

SS 2024-07-31 8 30 53

, which I believe is not related to my change. Probably I have missed something, but I couldn't find what I should do. Could you let me know what should I do, if you can guess?

@hoxbro
Copy link
Member

hoxbro commented Jul 31, 2024

You are only running the core-test; try running the full test suite with pixi run -e test-312 test-unit.

Also, make sure you have pulled in the latest changes made to this PR, e.g., I merged with the main branch to fix some compatibility issues.

@arafune
Copy link
Contributor Author

arafune commented Jul 31, 2024

@hoxbro

Thank you for your reply.

Unfortunately, I cannot still go well. (I did pixi install and pixi run download-data, they worked well.)

スクリーンショット 2024-07-31 16 22 40

Can -e test-312 really work on Mac ? Or I have sitll missed something?

@hoxbro
Copy link
Member

hoxbro commented Jul 31, 2024

Yes, they should be able to run on MacOS, but it seems like a bad file. Try rm -rf .pixi pixi.lock and then pixi run again.

Are you running on an ARM Mac? Or is it an Intel?

@arafune
Copy link
Contributor Author

arafune commented Jul 31, 2024

@hoxbro

Thank you for your help.

After removing .pixi and pixi.lock, pixie run works well.
I'll try to revise the PR. But the current simple PR would not sufficiently good.
Thus, I would like to close this PR. After revision (with test on my system) I'll request the PR once again.

Thank you.

@arafune arafune closed this Jul 31, 2024
@arafune arafune deleted the work_around_6327 branch July 31, 2024 09:05
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 12, 2024
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.

3 participants