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.

Sign up for GitHub

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

Jump to bottom

Remove now useless padding as it is now down automatically. #680

Closed
wants to merge 3 commits into from
Closed

Remove now useless padding as it is now down automatically. #680

wants to merge 3 commits into from

Conversation

nouiz
Copy link
Collaborator

@nouiz nouiz commented Feb 25, 2024

No description provided.

@nouiz nouiz added the jax label Feb 25, 2024
@nouiz
Copy link
Collaborator Author

nouiz commented Feb 25, 2024

/te-ci jax

2 similar comments
@nouiz
Copy link
Collaborator Author

nouiz commented Feb 25, 2024

/te-ci jax

@ksivaman
Copy link
Member

/te-ci jax

@ksivaman
Copy link
Member

@nouiz 0.4.16 was out on 18th September 2023, so with this change we're effectively supporting only 5 months of JAX releases, which seems too few

@denera
Copy link
Collaborator

denera commented Feb 28, 2024

/te-ci jax

@nouiz
Copy link
Collaborator Author

nouiz commented Feb 28, 2024

@nouiz 0.4.16 was out on 18th September 2023, so with this change we're effectively supporting only 5 months of JAX releases, which seems too few

Last summer, we where only supporting the last JAX release. That we now support 5 months is awesome!
But I think as the JAX release cycle is very fast, 5 months is too much.
I think 3 months (this is the normal JAX deprecation policy) is enough. But maybe in some cases, we won't be able to support that too.

I don't see 5 months as an issue blocking this PR. If you do, can you tell why?

ksivaman
ksivaman approved these changes Feb 28, 2024
View reviewed changes
Copy link
Member

@ksivaman ksivaman left a comment

Choose a reason for hiding this comment

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

LGTM

@nouiz nouiz force-pushed the get_padded_cleanup branch from b0c7832 to 05fe402 Compare March 6, 2024 20:57
@nouiz
Copy link
Collaborator Author

nouiz commented Mar 6, 2024

/te-ci jax

1 similar comment
@ksivaman
Copy link
Member

ksivaman commented Mar 7, 2024

/te-ci jax

@ksivaman
Copy link
Member

@nouiz Could you resolve the conflicts so that we can merge this?

@nouiz nouiz force-pushed the get_padded_cleanup branch from 05fe402 to 83d3e4a Compare March 19, 2024 03:37
@nouiz
Copy link
Collaborator Author

nouiz commented Mar 19, 2024

@nouiz Could you resolve the conflicts so that we can merge this?

done

@denera
Copy link
Collaborator

denera commented Mar 27, 2024

/te-ci jax

@ksivaman
Copy link
Member

After a discussion with @nouiz, we will re-open a fresh PR.

@ksivaman ksivaman closed this Jul 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ksivaman ksivaman ksivaman approved these changes

Assignees
No one assigned
Labels
jax
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@nouiz @ksivaman @denera