fill in ACE1pack #1

gelzinyte · 2022-01-27T18:52:39Z

A draft of a complete package. @casv2 (and anyone else if they wish!) could you please leave some early feedback on the work in progress? Anything missing, any suggestions? Some things need better names (e.g. everything.jl & fit_ace).

Meanwhile, I'll work on debugging, tests & CI, TODO's and docstrings in the code. These should be done before merging, but I have left some "enhancement" comments in the code that might be useful in slightly longer term.

cortner · 2022-01-28T00:16:40Z

Thank you, Elena! We need a brief discussion how to go about merging yours vs adapt to latest IPFitting first; cc @casv2 ; cf #2 ; I see you've already introduced that version bound though so maybe you are further with this than I thought.

casv2 · 2022-01-28T00:22:19Z

Yes, Elena and I have been coordinating quite closely in designing ACE1pack and IPFitting, and will continue to do so. Let's discuss tomorrow, I think we're quite close. Regarding the regularisation interface the idea is to put the ACE1.scaling oneliner into ACE1pack. This way we set up the preconditioning matrix in ACE1pack (which has the ACE dependency) and send it to IPFitting for fitting.

cortner · 2022-01-28T00:23:45Z

I'm ok with that.

gelzinyte · 2022-01-31T17:44:19Z

The TiAl tutorial now works - see test_fit.jl (which is more of a example/draft than an actual unit test, for now).

I'm working on the unit tests, but that can also done as another pull request if you think that it's better to merge and open this up for everyone as soon as possible.

Looking forward to any comments on the code!

cortner · 2022-01-31T17:46:52Z

so in principle you think it is ready to tag a first 0.0.1 version? and let people try it out?

gelzinyte · 2022-01-31T17:50:48Z

yes, I think so!

…cumentation

gelzinyte · 2022-02-01T16:49:15Z

The current tests mostly just call functions to make sure they execute without an error. I thought anything less trivial (mainly - that the objects constructed from given parameters are correct) would be tested within the underlying modules and that testing for specific outputs (e.g. that default values are as expected) seemed maybe excessive. Is there anything else that would be good to test for?

The tests on GitHub currently fail, has anyone stumbled upon anything similar?

On the x86 arch only: HDF5 isn't precompiled correctly
Tests on x64 arch do finish, but the errors of a TiAl fit are different from those I get when running the tests locally.

I think this should have everything that is needed at the first instance. I would eventually like to use multitransform and be able to specify polynomial degree for each correlation order, but I would first like to integrate ACE1pack with Workflow.

.github/workflows/CI.yml

src/basis.jl

gelzinyte · 2022-02-07T20:29:48Z

Thank you very much for all the feedback! I think I have followed up to all of your suggestions - looking forward to any other comments.

cortner · 2022-02-07T22:47:07Z

@gelzinyte you may want to put a lower version bound 0.11.4 for JuLIP so that the YAML works.

gelzinyte · 2022-02-14T19:04:06Z

@cortner do you have any further feedback? Maybe it would be good to merge and open this up to others and add any further changes as separate pull requests?

cortner · 2022-02-15T04:54:31Z

src/fit.jl

+    if typeof(params["e0"]) == Dict{Any, Any}
+        # sometimes gets read in (from yaml?) as Dict{Any, Any} 
+        # which gives StackOverflowError somewhere in OneBody
+        params["e0"] = convert(Dict{String, Any}, params["e0"])


sounds like it should be filed as an issue with JuLIP

cortner · 2022-02-15T04:57:36Z

@gelzinyte all looks good. Just need to confirm a licensing issue with @gabor1 and then we should be ready to merge. Thank you for taking the lead on this. I'll wait for your instruction though to first merge the data repo and then this here. We can try to wrap it up tomorrow (Tuesday).

cortner · 2022-02-15T05:06:27Z

hm .... I'm struggling with the Artifacts...

┌ Warning: using Pkg instead of using LazyArtifacts is deprecated
│   caller = ip:0x0
└ @ Core :-1
  Downloaded artifact: TiAl_tiny_dataset
 Downloading artifact: TiAl_tiny_dataset
┌ Error: Hash Mismatch!
│   Expected sha256:   42f96e718e7b8d1f3e9bbbe0b7c2be1b3bfe96c645373af5ac6eeebc997629e2
│   Calculated sha256: 5c5ddd4883d50d1f5ce8c6bacc36ec15ffd08d96be0b774fea7262db14e1ba25
└ @ Pkg.PlatformEngines ~/gits/julia17/usr/share/julia/stdlib/v1.7/Pkg/src/Plat
  Downloaded artifact: TiAl_tiny_dataset
ERROR: LoadError: Unable to automatically install 'TiAl_tiny_dataset' from '/Users/ortner/gits/ACE1pack/Artifacts.toml'
Stacktrace:

cortner · 2022-02-15T05:09:01Z

and even if I fix the sha and switch to LazyArtifacts, I get this:

  Downloaded artifact: TiAl_tiny_dataset
  Downloaded artifact: TiAl_tiny_dataset
ERROR: LoadError: Unable to automatically install 'TiAl_tiny_dataset' from '/Users/ortner/gits/ACE1pack/Artifacts.toml'
Stacktrace:
...

cortner · 2022-02-15T05:09:10Z

thoughts?

gelzinyte · 2022-02-15T21:40:02Z

I think the problem was that the line

tarball_hash = archive_artifact(hash_, joinpath(tarfile))

was modifying the tarfile and as a result, tarball_hash was different from the hash for the file at the url. But (url, tarball_hash) were bundled together. Which is why hash for file at downloaded didn't match the saved key. I didn't have this problem initially, because the files on my computer were downloaded with generate_artifacts.jl and weren't checked against not-yet-defined incorrect hash.

The problem appeared only after removing the appropriate directories under ~/.julia/artifacts and re-runing artifacts.jl to trigger a new download. I followed the ACE create_artifacts.jl, so that repo might have the sane problem?

My workaround was to generate the hash myself, although I'm surprised that I had to, so maybe something's not as it's supposed to be? Also, a (small?) caveat - this relies on any downloaded artefacts being already compressed or small.

Unfortunately, something's still wrong and the error message isn't very helpful:

(main) ➜  test git:(eg/dev) ✗ julia artifacts.jl 
  Downloaded artifact: TiAl_tiny_dataset
  Downloaded artifact: TiAl_tiny_dataset
ERROR: LoadError: Unable to automatically install 'TiAl_tiny_dataset' from '/Users/elena/.julia/dev/ACE1pack/Artifacts.toml'
Stacktrace:
 [1] error(s::String)
   @ Base ./error.jl:33
 [2] ensure_artifact_installed(name::String, meta::Dict{String, Any}, artifacts_toml::String; platform::Base.BinaryPlatforms.Platform, verbose::Bool, quiet_download::Bool, io::Base.TTY)
   @ Pkg.Artifacts /Applications/Julia-1.7.app/Contents/Resources/julia/share/julia/stdlib/v1.7/Pkg/src/Artifacts.jl:441
 [3] ensure_artifact_installed(name::String, artifacts_toml::String; platform::Base.BinaryPlatforms.Platform, pkg_uuid::Nothing, verbose::Bool, quiet_download::Bool, io::Base.TTY)
   @ Pkg.Artifacts /Applications/Julia-1.7.app/Contents/Resources/julia/share/julia/stdlib/v1.7/Pkg/src/Artifacts.jl:404
 [4] _artifact_str(__module__::Module, artifacts_toml::String, name::SubString{String}, path_tail::String, artifact_dict::Dict{String, Any}, hash::Base.SHA1, platform::Base.BinaryPlatforms.Platform, lazyartifacts::Any)
   @ Artifacts /Applications/Julia-1.7.app/Contents/Resources/julia/share/julia/stdlib/v1.7/Artifacts/src/Artifacts.jl:547
 [5] invokelatest(::Any, ::Any, ::Vararg{Any}; kwargs::Base.Pairs{Symbol, Union{}, Tuple{}, NamedTuple{(), Tuple{}}})
   @ Base ./essentials.jl:716
 [6] invokelatest(::Any, ::Any, ::Vararg{Any})
   @ Base ./essentials.jl:714
 [7] top-level scope
   @ /Applications/Julia-1.7.app/Contents/Resources/julia/share/julia/stdlib/v1.7/Artifacts/src/Artifacts.jl:680
in expression starting at /Users/elena/.julia/dev/ACE1pack/test/artifacts.jl:4

cortner · 2022-02-15T21:41:49Z

so we are still stuck here? ...

cortner · 2022-02-15T21:42:37Z

I thought the hash can be just anything (compatible with whatever keys they use...) but you are saying this is not the case?

gelzinyte · 2022-02-15T21:49:30Z

scripts/generate_artifacts.jl

+
+artifacts_toml = joinpath(pathof(ACE1pack)[1:end-16], "Artifacts.toml")
+
+function _get_sha256(filename)


Is this sound? Will return close the file?

Alternatively, I thought about

function _get_sha256(filename) open(filename) do f global hash = bytes2hex(sha256(f)) end return hash end

but it seemed wrong to make hash accessible across the script.

It wasn't clear to me what is the correct way to access anything derived from the file contents outside of the do block. Maybe in that case one should explicitly close() the file?

I'm afraid I have no memory about how this works. Have you checked what I did in ACE1.jl?

Artifacts.toml:

["acedata_v0.8.1"] git-tree-sha1 = "23a2e3400188336105a4e47c31cdb7ce95a104cb" lazy = true [["acedata_v0.8.1".download]] sha256 = "91305df8a0b0572630b9be4b2f3b7e16fc14b2683182e25f18a338036eac0305" url = "https://github.com/ACEsuit/ACEData/archive/v0.8.1.tar.gz"

and then in artifacts.jl:

using Pkg.Artifacts datadir = joinpath(artifact"acedata_v0.8.1", "ACEData-0.8.1") testsdir = joinpath(datadir, "tests") traindir = joinpath(datadir, "trainingsets") potsdir = joinpath(datadir, "potentials") # unzip the eam potential if !isfile(joinpath(traindir, "w_eam4.fs")) eamzip = joinpath(traindir, "w_eam4.fs.zip") run(`unzip $eamzip -d $traindir`) end

why do you even need the sha in artifacts.jl?

why do you even need the sha in artifacts.jl?

bind_artifact!() needs both sha and url and you need the url to get the artefact.

I thought the hash can be just anything (compatible with whatever keys they use...) but you are saying this is not the case?

hashes for the downloaded file and file used to create the artefact need to match, no? I thought the initial error was raised because they didn't.

ACE1 artefacts are successfully downloaded with what I thought was equivalent code, so maybe the issue is with how the original artefact files were set up. I'll see if I can make any progress tomorrow

gelzinyte · 2022-02-17T12:25:16Z

The issues with artefacts only relate to tests. Since there is another PR coming to fix the GitHub's CI, what do you think about fixing the artefacts there as well?

cortner · 2022-02-17T15:44:57Z

ok, then I'll merge and tag a first version. We can go back to all remaining issues afterwards.

cortner · 2022-02-17T15:45:17Z

thank you again for all your work on this.

cortner · 2022-02-17T15:50:41Z

done, this is tagged as 0.0.1

Root cause: This is a fork repository (jameskermode/ACEpotentials.jl) forked from upstream (ACEsuit/ACEpotentials.jl). GitHub Actions don't run automatically on forked repositories due to security restrictions. This is standard GitHub behavior to prevent: - Malicious code execution - CI minute abuse - Secrets exposure - Resource waste Solutions documented: 1. Enable GitHub Actions in fork settings (recommended) 2. Manual workflow dispatch 3. Create PR to upstream instead 4. Test locally with Julia See FORK_CI_ISSUE.md for detailed analysis and step-by-step fixes. Related: PR #1 at jameskermode#1

…-011CV4PfXPf4MHS4ceHdoMQe Migrate from EquivariantModels.jl to EquivariantTensors.jl

gelzinyte added 2 commits January 27, 2022 10:39

updated IPFfitting dependency to v0.10

4cf7c39

first draft of the full package

56d0b6f

gelzinyte added 3 commits January 31, 2022 17:22

debugged

a4bdb3f

primitive example

5ab296c

updated info messages

91479f2

gelzinyte changed the title ~~WIP: fill in ACE1pack~~ fill in ACE1pack Jan 31, 2022

gelzinyte added 11 commits January 31, 2022 21:50

updated tests

1daf78c

add MolSim registry to github CI

e76faa4

alternative way of adding MolSim registry

5739cbb

bug in CI.yaml

8b6586a

install ase

35c6015

debug hdf5

b826439

alternative precompile for debugging HDF5; installing Python for do…

0efc8d6

…cumentation

extra quick JSON reader

341f359

reading (required) parameters from json or yaml

b30798a

remove excessive "params" and "dict"

945b5cc

different default radial basis behavirour for pair and rpi basis

0c0052b

try to add Python without installing ASE

278b5e6

gelzinyte commented Feb 1, 2022

View reviewed changes

.github/workflows/CI.yml Outdated Show resolved Hide resolved

re-introduce installing ASE from Python

129f2bb

gelzinyte commented Feb 1, 2022

View reviewed changes

src/basis.jl Outdated Show resolved Hide resolved

gelzinyte commented Feb 1, 2022

View reviewed changes

src/basis.jl Outdated Show resolved Hide resolved

remove minor comments

5577d67

gelzinyte added 7 commits February 8, 2022 10:37

fixed JuLIP to 0.11.4

70e12ae

updated params["basis"] to be a dictionary

711ca11

reading from yaml too

82f2c4f

parameters entry for saving lsq database

608e7df

typo

1c640e8

changed to specifying full ACE filename

493cd50

not appending redundant .json suffix when saving the potential

605d478

cortner reviewed Feb 15, 2022

View reviewed changes

gelzinyte added 2 commits February 15, 2022 21:16

change what hash gets saved for the url file

3f5b019

Pkg.Artifacts -> LazyArtifacts

a5e7847

gelzinyte commented Feb 15, 2022

View reviewed changes

cortner merged commit b1505b8 into main Feb 17, 2022

gelzinyte mentioned this pull request Mar 9, 2022

Generate artefacts #14

Merged

wcwitt deleted the eg/dev branch August 19, 2023 15:56

cortner pushed a commit that referenced this pull request Nov 15, 2025

Merge pull request #1 from jameskermode/claude/explore-repo-structure…

65185fc

…-011CV4PfXPf4MHS4ceHdoMQe Migrate from EquivariantModels.jl to EquivariantTensors.jl


		artifacts_toml = joinpath(pathof(ACE1pack)[1:end-16], "Artifacts.toml")

		function _get_sha256(filename)

fill in ACE1pack #1

fill in ACE1pack #1

Uh oh!

Conversation

gelzinyte commented Jan 27, 2022

Uh oh!

cortner commented Jan 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

casv2 commented Jan 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cortner commented Jan 28, 2022

Uh oh!

gelzinyte commented Jan 31, 2022

Uh oh!

cortner commented Jan 31, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gelzinyte commented Jan 31, 2022

Uh oh!

gelzinyte commented Feb 1, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gelzinyte commented Feb 7, 2022

Uh oh!

cortner commented Feb 7, 2022

Uh oh!

gelzinyte commented Feb 14, 2022

Uh oh!

cortner Feb 15, 2022

Choose a reason for hiding this comment

Uh oh!

cortner commented Feb 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cortner commented Feb 15, 2022

Uh oh!

cortner commented Feb 15, 2022

Uh oh!

cortner commented Feb 15, 2022

Uh oh!

gelzinyte commented Feb 15, 2022

Uh oh!

cortner commented Feb 15, 2022

Uh oh!

cortner commented Feb 15, 2022

Uh oh!

gelzinyte Feb 15, 2022

Choose a reason for hiding this comment

Uh oh!

cortner Feb 15, 2022

Choose a reason for hiding this comment

Uh oh!

cortner Feb 15, 2022

Choose a reason for hiding this comment

Uh oh!

cortner Feb 15, 2022

Choose a reason for hiding this comment

Uh oh!

gelzinyte Feb 15, 2022

Choose a reason for hiding this comment

Uh oh!

gelzinyte commented Feb 17, 2022

Uh oh!

cortner commented Feb 17, 2022

Uh oh!

cortner commented Feb 17, 2022

Uh oh!

cortner commented Feb 17, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

cortner commented Jan 28, 2022 •

edited

Loading

casv2 commented Jan 28, 2022 •

edited

Loading

cortner commented Jan 31, 2022 •

edited

Loading

cortner commented Feb 15, 2022 •

edited

Loading