Conversation
|
Please merge with devel. |
Codecov Report
@@ Coverage Diff @@
## multi-task #929 +/- ##
===============================================
- Coverage 75.41% 64.28% -11.14%
===============================================
Files 85 5 -80
Lines 6729 14 -6715
===============================================
- Hits 5075 9 -5066
+ Misses 1654 5 -1649 Continue to review full report at Codecov.
|
|
err... We also need to update the target branch. |
deepmd/train/trainer_mt.py
Outdated
|
|
||
|
|
||
|
|
||
| class DPTrainer_mt (object): |
There was a problem hiding this comment.
I think this class can inherit DPTrainer and remove duplicate methods.
There was a problem hiding this comment.
And I still keep this advice: this class can inherit DPTrainer and remove duplicate methods. Or you can consider to create a new class DPTrainerBase to merge the same methods.
njzjz
left a comment
njzjz
left a comment
There was a problem hiding this comment.
Not all previous comments are resolved.
deepmd/utils/argcheck_mt.py
Outdated
| doc_descrpt_type = f'The type of the descritpor. See explanation below. \n\n\ | ||
| - `loc_frame`: Defines a local frame at each atom, and the compute the descriptor as local coordinates under this frame.\n\n\ | ||
| - `se_e2_a`: Used by the smooth edition of Deep Potential. The full relative coordinates are used to construct the descriptor.\n\n\ | ||
| - `se_e2_r`: Used by the smooth edition of Deep Potential. Only the distance between atoms is used to construct the descriptor.\n\n\ | ||
| - `se_e3`: Used by the smooth edition of Deep Potential. The full relative coordinates are used to construct the descriptor. Three-body embedding will be used by this descriptor.\n\n\ | ||
| - `se_a_tpe`: Used by the smooth edition of Deep Potential. The full relative coordinates are used to construct the descriptor. Type embedding will be used by this descriptor.\n\n\ | ||
| - `hybrid`: Concatenate of a list of descriptors as a new descriptor.' |
There was a problem hiding this comment.
se_conv1d and se_conv_geo are not added here.
There was a problem hiding this comment.
I think I can deal with the argcheck problem.
As for inheriting DPTrainer, I definitely think there's a must to do it. However, there are not much duplicate method we can remove, because each build and training process is modified and DPTrainer doesn't split some big function into small ones, which means I can't apply them, instead I need to go through like the whole _init_param to change some details
deepmd/train/trainer_mt.py
Outdated
|
|
||
|
|
||
|
|
||
| class DPTrainer_mt (object): |
There was a problem hiding this comment.
And I still keep this advice: this class can inherit DPTrainer and remove duplicate methods. Or you can consider to create a new class DPTrainerBase to merge the same methods.
|
It is definitely not a good practice to write commit messages like "add multi-task v4". There are many instructions on how to write good commit message for example. Please describe what you have done in the description of this PR. Good examples are #945 #921 Please follow them to update your PR. Thanks! |
amcadmus
left a comment
amcadmus
left a comment
There was a problem hiding this comment.
- other
fitandloss(deep tensor) should be added with attributename - I am not sure if the data
source/tests/multi-task/data/is necessary for your test cases, please check.
deepmd/entrypoints/train.py
Outdated
| log_level=log_level, | ||
| mpi_log=mpi_log | ||
| mpi_log=mpi_log, | ||
| try_distrib=jdata.get("with_distrib", False), |
There was a problem hiding this comment.
This would not pass, RunOptions does not take try_distrib anymore
deepmd/entrypoints/train.py
Outdated
|
|
||
|
|
There was a problem hiding this comment.
do not add blank lines to the code.
deepmd/model/ener.py
Outdated
| all_stat['box'], | ||
| all_stat['type'], | ||
| all_stat['natoms_vec'], | ||
| all_stat['natoms_vec'], # this should be the global one(dim 14) or local one(dim 11)? |
There was a problem hiding this comment.
I'm thinking about how to organize data more reasonably, because now we are using global type map everywhere, and each descriptor deal with its own atom type
deepmd/train/run_options.py
Outdated
| log_level: int = 0, | ||
| mpi_log: str = "master" | ||
| mpi_log: str = "master", | ||
| try_distrib: bool = False, |
There was a problem hiding this comment.
This is not added by me, I merged the devel (maybe) and there are some inconsistence here in run options
There was a problem hiding this comment.
I see that may because that you didn't solve the conflict correctly.
|
|
||
|
|
||
| def valid_on_the_fly(self, | ||
| fp, |
There was a problem hiding this comment.
the fp parameter is not used anymore , pleas remove.
There was a problem hiding this comment.
I check out the devel branch, but fp is not deprecated?
| self.type_map = type_map | ||
| ntypes = len(type_map) | ||
| self.type_map = type_map[:ntypes] | ||
|
|
There was a problem hiding this comment.
You are reverting bug fixings!!
deepmd/utils/data.py
Outdated
| def get_data_dict(self) -> dict: | ||
| def get_data_dict(self) -> (dict, str): | ||
| """ | ||
| Get the `data_dict` | ||
| """ | ||
| return self.data_dict | ||
| return self.data_dict, self.name |
There was a problem hiding this comment.
You already provide the get_name method, I think it is not necessary to return self.name by get_data_dict
deepmd/utils/data_system.py
Outdated
| #print("self.type_map",self.type_map) | ||
| #print("type_map",type_map) |
deepmd/utils/data_system.py
Outdated
| return b_data | ||
|
|
||
| return b_data |
deepmd/utils/data_system.py
Outdated
| return sys_probs | ||
|
|
||
|
|
||
| class DeepmdDataDocker() : |
There was a problem hiding this comment.
Should be implemented in a separate file.
deepmd/entrypoints/train.py
Outdated
| for model_name in model.model_dict.keys(): | ||
| sub_model = model.model_dict[model_name] | ||
| rcut_list.append(sub_model.get_rcut()) | ||
| type_map = sub_model.get_type_map() |
There was a problem hiding this comment.
they are the same for now
examples/mix/input.json
Outdated
| }, | ||
| "descriptor" :[ | ||
| { | ||
| "type": "se_e2_a", |
There was a problem hiding this comment.
Please take care of indents.
|
Sorry, to call the multi task training, it is still using 'dp train -mt True input.json', however train and train_mt is separated |
I see |
deepmd/entrypoints/main.py
Outdated
| type=bool, | ||
| default=False, |
There was a problem hiding this comment.
I suggest to use action="store_true" instead.
| type=bool, | |
| default=False, | |
| action="store_true", |
amcadmus
left a comment
amcadmus
left a comment
There was a problem hiding this comment.
plz revert all changes on adding/removing blank lines.
deepmd/entrypoints/train.py
Outdated
| mpi_log: str, | ||
| log_level: int, | ||
| log_path: Optional[str], | ||
| multi_task: bool, |
There was a problem hiding this comment.
Why do we still have the option in train?
deepmd/entrypoints/train.py
Outdated
| from deepmd.train.trainer_mt import DPMultitaskTrainer | ||
| from deepmd.utils.argcheck import normalize | ||
| from deepmd.utils.argcheck_mt import normalize_mt | ||
| from deepmd.utils.compat import updata_deepmd_input | ||
| from deepmd.utils.data_system import DeepmdDataSystem | ||
| from deepmd.utils.data_docker import DeepmdDataDocker |
There was a problem hiding this comment.
These imports should not be necessary
| def get_rcut(jdata): | ||
| descrpt_data = jdata['model']['descriptor'] | ||
| def parse_rcut(descrpt_data): | ||
| rcut_list = [] | ||
| if descrpt_data['type'] == 'hybrid': | ||
| for ii in descrpt_data['list']: | ||
| rcut_list.append(ii['rcut']) | ||
| else: | ||
| rcut_list.append(descrpt_data['rcut']) | ||
| return rcut_list | ||
|
|
||
| def get_rcut(jdata): | ||
| descrpt_data = jdata['model']['descriptor'] | ||
| rcut_list = [] | ||
| rcut_list.extend(parse_rcut(descrpt_data)) |
There was a problem hiding this comment.
I do not think you need to revise the get_rcut.
There was a problem hiding this comment.
if I don't split get_rcut into two function, I can't reuse it in the train_mt
deepmd/model/ener.py
Outdated
| all_stat['box'], | ||
| all_stat['type'], | ||
| all_stat['natoms_vec'], | ||
| all_stat['natoms_vec'], |
There was a problem hiding this comment.
plz avoid introduce such revision.
deepmd/utils/data_system.py
Outdated
| b_data["natoms_vec"] = self.natoms_vec[self.pick_idx] | ||
| b_data["default_mesh"] = self.default_mesh[self.pick_idx] | ||
| return b_data | ||
| return b_data |
|
|
||
|
|
deepmd/model/ener.py
Outdated
| mesh, | ||
| input_dict, | ||
| suffix = '', | ||
| suffix, # a dict of suffix, including type_embed, descrpt and fitting. |
There was a problem hiding this comment.
I think we should preserve the default value ''
source/tests/test_multitask.py
Outdated
| dd = dd.reshape([3,-1]) | ||
| ref_loss = [24.8,19.6, | ||
| 3.79,1.81, | ||
| 2110.0,2110.0] | ||
|
|
||
| for ii in range(3): | ||
| for jj in range(2): | ||
| self.assertAlmostEqual(dd[ii][jj+1], ref_loss[ii*2+jj], places = 8) |
There was a problem hiding this comment.
why only compare the total loss?
5 participants
No description provided.