bugfix: processor device, in-place ops and more#7
Conversation
saraheisenach
left a comment
saraheisenach
left a comment
There was a problem hiding this comment.
Left some comments/ideas (nothing crazy, just mainly style suggestions haha) :)
| if device is None: | ||
| try: | ||
| self.data, self.slices = torch.load(self.processed_paths[0]) | ||
| except: | ||
| self.data, self.slices = torch.load(self.processed_paths[0], map_location=torch.device('cpu')) | ||
| else: | ||
| if device == 'cpu': | ||
| self.data, self.slices = torch.load(self.processed_paths[0], map_location=torch.device(device)) | ||
| else: | ||
| self.data, self.slices = torch.load(self.processed_paths[0]) |
There was a problem hiding this comment.
I'm not sure I fully understand this logic. Wouldn't this behavior be the same?
| if device is None: | |
| try: | |
| self.data, self.slices = torch.load(self.processed_paths[0]) | |
| except: | |
| self.data, self.slices = torch.load(self.processed_paths[0], map_location=torch.device('cpu')) | |
| else: | |
| if device == 'cpu': | |
| self.data, self.slices = torch.load(self.processed_paths[0], map_location=torch.device(device)) | |
| else: | |
| self.data, self.slices = torch.load(self.processed_paths[0]) | |
| try: | |
| self.data, self.slices = torch.load(self.processed_paths[0], map_location=torch.device(device)) | |
| except: | |
| self.data, self.slices = torch.load(self.processed_paths[0], map_location=torch.device('cpu')) | |
If map_location is None, that is the same as the default value, so it shouldn't matter whether it is passed. And then the fallback is just if whatever device is passed in doesn't work, then default to cpu, right?
Also, we should try to specify the exception that's thrown whenever possible, so in this case, do you know what error would be thrown? If not we can leave it, but it's nice to have
There was a problem hiding this comment.
There might be a situation in which GPU is available but the dataset needs to be loaded on CPU. I have re-implemented the logic to make it clearer.
|
|
||
| def threshold_sort(all_distances, r, n_neighbors): | ||
| A = all_distances.clone().detach() | ||
| # A = all_distances.clone().detach() |
There was a problem hiding this comment.
Do we still need this commented out line?
There was a problem hiding this comment.
I think we can leave it there for the time being.
| try: | ||
| delattr(data, attr) | ||
| except AttributeError: | ||
| except: |
There was a problem hiding this comment.
Like I said above, if possible, we should try to keep the specific exceptions in our try except blocks, so if there's another exception you think is needed, you could doexcept (AttributeError, <other exception>):
There was a problem hiding this comment.
Reimplemented this part to do without try and except blocks.
| # fill in the original values | ||
| self_loop_diag = distance_matrix.diagonal() | ||
| cutoff_distance_matrix.diagonal().copy_(self_loop_diag) | ||
| # if image_selfloop: |
There was a problem hiding this comment.
Same here, is this something that will be implemented later or can we remove it? If we need it, maybe add a TODO saying what needs to be done?
There was a problem hiding this comment.
I think we can leave it there for the time being.
| self.disable_tqdm = logging.root.level > logging.INFO | ||
| self.device = "cpu" | ||
|
|
||
| def set_device(self, device): |
There was a problem hiding this comment.
Is this function called anywhere? Or is the device now always cpu?
There was a problem hiding this comment.
device should be cpu by default. I have removed the setter method to make device as an input parameter with default value cpu.
| elif isinstance(s["y"], list): | ||
| _y = [float(each) for each in s["y"]] | ||
| y.append(_y) | ||
| y_dim = len(_y) |
There was a problem hiding this comment.
y_dim is the same for every s['y'] in the same run, right? Could you maybe move y_dim assignment from line 212 and from 240 to right above 244 outside of the for loop just to keep them together? Like maybe (or something a little less messy):
y_dim = len(original_structures[0]['y']) if isinstance(original_structures[0]['y'], list) else 1
To me, it makes more sense to keep assignments together so it's easier to read, but no pressure to move it if you feel differently!
There was a problem hiding this comment.
Thanks for the suggestion! Accepted your change.
3 participants
This merge fixes/updates the following:
data.ptfiles now handles both cpu and gpu cases.tqdmprogress bar silent iflogging.root.level > logging.INFO.Tested on NERSC on
STO_DOS_dataandMP_data_npj.