Skip to content

Remove type constraints for copy of ArrayPartition - #276

Merged
ChrisRackauckas merged 5 commits into
SciML:masterfrom
DrChainsaw:copychtype
Sep 8, 2023
Merged

Remove type constraints for copy of ArrayPartition#276
ChrisRackauckas merged 5 commits into
SciML:masterfrom
DrChainsaw:copychtype

Conversation

@DrChainsaw

@DrChainsaw DrChainsaw commented Sep 4, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

Fix #275

Seems like it was ok to just remove all type constraints. I did a quick check with a dummy array type and the performance was identical to current main.

I tried to do the same thing for zero, but then it would no longer infer (even with the same construction as in the example in #275). Since I don't seem to need it for my use case I just left it as is.

Also did the same for zero since it had the same type of issue.

The tests might seem a bit silly. I put it in a separate commit in case you don't want it.

@DrChainsaw

DrChainsaw commented Sep 4, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Just realized current version does not infer correctly for nested ArrayPartitions. Changing the broadcast to map seems to fix this. Running tests locally atm to verify that nothing else breaks from this.

Current version should be ok now.

@DrChainsaw

DrChainsaw commented Sep 5, 2023

Copy link
Copy Markdown
Contributor Author

I tried running the formatter like this:

julia> using JuliaFormatter

julia> format_file("src/array_partition.jl"; style=SciMLStyle())
false

But it did alot of changes to the file and did not touch the lines changed by this PR so I didn't commit it. I also tried with the default style with the same result.

Is there some other way to run it or is the file unformatted on current main?

@ChrisRackauckas

ChrisRackauckas commented Sep 6, 2023

Copy link
Copy Markdown
Member

The format changed. I wouldn't worry about that. I'm going to fix formatter things in the near future.

https://github.com/SciML/RecursiveArrayTools.jl/actions/runs/6076983375/job/16487010044?pr=276#step:6:414

It seems you branched off of an earlier master that fixed the Project.toml form though. Can you try rebasing?

@DrChainsaw

DrChainsaw commented Sep 7, 2023

Copy link
Copy Markdown
Contributor Author

It seems you branched off of an earlier master that fixed the Project.toml form though. Can you try rebasing?

Hmmm, I can't seem to find any missing commit on the branch. Below is the last one which is not mine.

commit 6e8600fe96a10112a59e146051d29399370a09b7 (origin/master, origin/HEAD, master)   
Merge: d06ecb8 727a4b6
Author: Christopher Rackauckas <[email protected]>
Date:   Mon Aug 21 19:04:16 2023 +0200

    Merge pull request #272 from visr/visr-patch-1
    
    fix UndefVarError: `_throw_dmrs` not defined

@ChrisRackauckas
ChrisRackauckas merged commit 49f3181 into SciML:master Sep 8, 2023
@ChrisRackauckas

ChrisRackauckas commented Sep 9, 2023

Copy link
Copy Markdown
Member

Seems like it was just an aqua issue on v1.6 so I disabled it for earlier julia

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ChrisRackauckas ChrisRackauckas ChrisRackauckas approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

See More

Successfully merging this pull request may close these issues.

Loosen type constraints for ArrayPartition copy?

2 participants

@DrChainsaw @ChrisRackauckas