search

nomad-coe / electronic-parsers

Apache License 2.0
18 stars 7 forks source link

TBStudio parser #154

Closed JosePizarro3 closed 7 months ago

mohammadnakhaee commented 10 months ago

Hi @JosePizarro3 I added the test for the parser. Please take a look if we can merge it.

mohammadnakhaee commented 10 months ago

Hi @JosePizarro3 Please take a look on the last changes.

JosePizarro3 commented 8 months ago

@mohammadnakhaee let me know when you need another quick round of review. I'll like to merge soon the new refactoring of 'Projection' to 'TB', https://gitlab.mpcdf.mpg.de/nomad-lab/nomad-FAIR/-/merge_requests/1285.

Maybe, we should decouple both changes: one merge for the renaming, another merge for adding SlaterKoster and TBStudio.

mohammadnakhaee commented 8 months ago

@JosePizarro3 yes. Could you please take a look. I solved the problems and lint errors related to tbstudio but it is still failed. Maybe you know what is going wrong. The failed tests are not related to the tbstudio.

JosePizarro3 commented 8 months ago

@JosePizarro3 yes. Could you please take a look. I solved the problems and lint errors related to tbstudio but it is still failed. Maybe you know what is going wrong. The failed tests are not related to the tbstudio.

Maybe rebasing would solve the issue?

Bear in mind that pytest will anyways fail. I'd say that if after rebasing pylint and mypy do not pass, I'll take a look 🙂

mohammadnakhaee commented 8 months ago

Yeah let me rebase it. Maybe it helps.

mohammadnakhaee commented 8 months ago

I rebased it. Now the problem is only these two

electronicparsers/utils/utils.py:30:0: E0611: No name 'TB' in module 'nomad.datamodel.metainfo.simulation.workflow' (no-name-in-module) electronicparsers/utils/utils.py:30:0: E0611: No name 'TBMethod' in module 'nomad.datamodel.metainfo.simulation.workflow' (no-name-in-module)

which I guess it makes sense. Doesn't it? They are newly added in the gitlab branch which is not merged yet. Is it correct?

JosePizarro3 commented 8 months ago

which I guess it makes sense. Doesn't it? They are newly added in the gitlab branch which is not merged yet. Is it correct?

Yes, indeed 🙂

Perfect, I will take a look and give you some final review. Though it looked good at first sight. Do you want me also to review now the gitlab side?

mohammadnakhaee commented 8 months ago

Yes please see the gitlab again. I will close all the threads since there are extensive changes to the previous version. Would be nice if you comment on this version.

JosePizarro3 commented 8 months ago

@mohammadnakhaee all good, come to my office (I have a final doubt) and we do the final merge together 🙂