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

Skeleton updates #3

Merged
merged 6 commits into from
Dec 11, 2024
Merged

Skeleton updates #3

merged 6 commits into from
Dec 11, 2024

Conversation

lkdvos
Copy link
Contributor

@lkdvos lkdvos commented Dec 10, 2024

This PR brings the package up to date with the skeleton v0.2.0

Copy link

codecov bot commented Dec 10, 2024

Welcome to Codecov 🎉

Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests.

Thanks for integrating Codecov - We've got you covered ☂️

@lkdvos lkdvos mentioned this pull request Dec 11, 2024
@lkdvos
Copy link
Contributor Author

lkdvos commented Dec 11, 2024

ping @mtfishman

@mtfishman
Copy link
Member

mtfishman commented Dec 11, 2024

@lkdvos I've started to prefer the style of having the using statements at the top of the files where they are being used, rather than in a long list at the top of the module. (Note that ExplicitImports.jl can help with finding the file locations.)

@lkdvos
Copy link
Contributor Author

lkdvos commented Dec 11, 2024

I understand the benefit, although my main reason against this is that these using statements leak out of the file, so that it becomes very hard to track the namespace of each of the files. You would have to either be quite diligent, and manually make sure that you repeat the imported names in each file, or check all of the preceding files to find out where a name is coming from.
Knowing myself, I usually will not double check if I added the required imports if the code runs, which is why I find it a lot more convenient to put the imports at the beginning of a module, so the namespace is the same for the entire module, and doesn't depend as much on where in the module you are.

I don't have too much experience with that package, how does it do if you are using the same imported name in multiple different files?

@mtfishman
Copy link
Member

mtfishman commented Dec 11, 2024

That's definitely a disadvantage, I guess I just have the attitude that it doesn't have to be perfect but at least it gives more useful information than having them all in a big list at the top level, and we can occasionally go and fix them up by commenting them all out and running ExplicitImports.jl with show_locations=true, which shows the files of where a name is used (it will show all of the file locations, if that is what you are asking). Also, if you reorganize the files and find something wasn't imported yet, then you can add the missing import, which isn't a big deal. And you can always get a full list of imported names across the packages with grep -rI "using".

So, my summary is that the current system is imperfect but still better than putting all using statements at the top level, and could be made more perfect with some tooling, say a CI check based on ExplicitImports.jl, a tool for making a PR to fill in missing using statements, etc.

@lkdvos
Copy link
Contributor Author

lkdvos commented Dec 11, 2024

I would say that having them all in one list at the top is more convenient, because you know there's a single place to go looking for the imports, and scrolling up to the top is not that much less work than switching to the module file.
That being said, this is not the hill I want to die on, so if you prefer it that way I'll change it.

@mtfishman
Copy link
Member

I would say that having them all in one list at the top is more convenient, because you know there's a single place to go looking for the imports, and scrolling up to the top is not that much less work than switching to the module file. That being said, this is not the hill I want to die on, so if you prefer it that way I'll change it.

Let's keep the current style for now and we can reassess as we go forward. We started with top level imports but recently switched to this new file-based style and in practice I like it better since it is easier to see locally where functions/types are coming from.

@mtfishman
Copy link
Member

For some context, this is the style used in Python, you are really forced to do it this way since each file is automatically a module. That design can be a bit annoying but it forces more discipline about namespaces.

@mtfishman mtfishman merged commit 8767c22 into ITensor:main Dec 11, 2024
11 checks passed
@lkdvos lkdvos deleted the skeleton branch December 11, 2024 21:49
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.

2 participants