Skip to content

Conversation

@geonho42
Copy link

Apply space group for MOF edges.

@Sangwon91
Copy link
Owner

한 번에 많은 양의 코드를 리뷰해야 하다보니 효율이 좀 떨어져서 일단은 한 번 풀 리퀘스트를 억셉하여 dev에 머지할 계획입니다. 그 이후에 추가적인 수정을 하는 부분이 좋을 것 같습니다. 이를 위해 2가지가 만족되면 좋을 것 같은데 다음과 같습니다.

  1. 기존 사용자의 코드 호환성을 위하여 기존 코드가 동일하게 작동하면 좋겠습니다. 예를 들어 builder에 필수 인자가 추가되면 버전 업데이트를 하면 작동하던 코드가 버전 업그레이드를 한 뒤 작동하지 않는 경우가 생깁니다. 이 부분이 좀 우려됩니다. 더 나은 개선이지만 유저가 선택적으로 작동하게 하면 좋겠고 차후에 major 버전 업데이트 할 때는 기본 옵션으로 적용하는 것이 좋아보입니다. 아예 독립된 새로운 빌더 class를 만드는 것도 한 방법입니다.
  2. 해당 기능 작동에 대한 설명에 대한 README.md 파일이 있으면 좋겠습니다. 아무 생각 없이 그냥 사용하는 기능은 아닌 것 같아 추가적인 README.md 필요합니다. main repository의 README.md는 이미 충분히 긴 상태라서 분리된 README.md 파일을 적절한 폴더에 생성하여 작성해주세요. 하나의 예시를 들면 decomposer의 README는 PORMAKE/pormake/experimental /decomposer/README.md에 위치합니다.

2번의 경우 저도 읽어봐야 생산적인 리뷰가 가능할 것 같습니다. 비교적 코드를 잘 이해하는 편인데 지금 변경된 내용이 많고 변동 사항이 보는 사람 관점에서는 직관적이지 않아 설명이 있으면 좋겠습니다.

@geonho42
Copy link
Author

geonho42 commented Sep 9, 2024

  1. 기존 코드와 호환되도록 input, output 조정 하도록 하겠습니다.
  2. 코딩을 시작할 때 데이터베이스 만든 후에 더 사용할 일이 없을 것이라고 생각하고 코딩해서 다른 사람이 보기에 불편한 것 같습니다. readme 파일 만들어서 추가하도록 하겠습니다.

@geonho42
Copy link
Author

geonho42 commented Sep 24, 2024

framework class에 min_array를 추가하여, 기존 코드와 완전히 호환되도록 변경하였습니다.
Readme 파일도 작성하여 추가하겠습니다.

@geonho42
Copy link
Author

edge rotation bug fix

@geonho42
Copy link
Author

geonho42 commented Oct 7, 2024

log file 자세하게 변경

@Sangwon91
Copy link
Owner

혹시 제가 README 파일 업로드를 발견 못한건가요? 새로 업로드 된 README.md 파일을 못찾겠네요

@geonho42
Copy link
Author

세미나 발표 하고 고칠 점들을 좀 수정하느라 늦어졌습니다. 빨리 작성하여 올리도록 하겠습니다.

@geonho42
Copy link
Author

energy 측정 방법 변경

@geonho42
Copy link
Author

README_PORMAKE v2.md 파일을 script 폴더에 업로드하였습니다.

@geonho42
Copy link
Author

Single metal node MOF 제작을 위한 building blocks 402개를 추가하였습니다.
script 폴더 안에 pormake_v2 폴더를 만들었습니다.
폴더 안에 README.md 파일을 이름 변경하여 이동하였습니다.

@Sangwon91
Copy link
Owner

작업하시느라 고생하셨습니다. 아쉽지만 지금 작성된 README가 충분하진 않고, 지금까지의 기능도 유기적으로 PORMAKE의 정식버전으로 확장될 수 있을지 모르겠습니다. 그럼에도 이러한 기능들이 의미 있고 또한 필요한 사람들이 있을 수 있으니 다음과 같은 제안을 드립니다.

  • 메인 README에 건호님의 fork주소의 존재와 그 기능에 대하여 간단하게 설명 *

또한 지금 확장은 제가 지속적으로 관리하기 어렵기 때문에 독립적인 fork로 관리되어야 할 것 같습니다. 가능하면 정식 버전에 당장 포함되었으면 했지만 아쉽게도 당장은 힘들 것 같습니다. 앞으로의 메인 PORMAKE의 변경을 고려하셔도 좋고 독립적인 개발을 하셔도 좋습니다.

@geonho42
Copy link
Author

감사합니다.

혹시 연결 링크 주소를 제 fork의 main branch로 바꾸어 주실 수 있으실까요? 제 main branch에 merge를 하였습니다.
그리고 가능하시다면 아래 내용 추가를 부탁드려도 될까요?
In addition, single metal node building blocks are added as an alternative to cluster building blocks, and corresponding organic linkers and organic node building blocks are added to generate porous materials with small pore sizes.

도와주셔서 감사합니다. 부족한 부분은 더 개선시켜 보도록 하겠습니다.

@Sangwon91
Copy link
Owner

네 요청하신 부분 적용하여 업데이트했습니다. https://github.com/geonho42/PORMAKE/blob/main/example/script/pormake_v2/README.md 파일같은 경우 이미지 경로가 잘못되어 표시가 안 되는 것 같은데 확인해보시면 좋을 것 같습니다. 고생하셨습니다~!

@geonho42
Copy link
Author

readme image 경로 수정

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.

3 participants