add dataset and dataloader for sample points #195
Conversation
dario-coscia
added
pr-to-fix
ndem0
added
pr-to-review
| @@ -135,8 +148,8 @@ def training_step(self, batch, batch_idx): | |||
| total_loss = sum(condition_losses) | |||
|
|
|||
| self.log('mean_loss', float(total_loss / len(condition_losses)), prog_bar=True, logger=True) | |||
| for condition_loss, loss in zip(condition_names, condition_losses): | |||
| self.log(condition_loss + '_loss', float(loss), prog_bar=True, logger=True) | |||
| # for condition_loss, loss in zip(condition_names, condition_losses): | |||
There was a problem hiding this comment.
why did we remove these loggers?
There was a problem hiding this comment.
Depending on the batch, you may not have all conditions at any iterations. It is something to fix once we clean the training structure
There was a problem hiding this comment.
Ok, we can log at each full dataloader iteration by taking the mean fo the batches losses. Let's fix it later
| ground_truth = batch['output'][condition_idx == condition_id] | ||
| loss = self._loss_data(samples, ground_truth) | ||
| else: | ||
| raise ValueError("Batch size not supported") |
There was a problem hiding this comment.
I am not sure what is going on here. Can you give me more details? Do you think we can make it easier so someone who is building a solver doesn't have to play with len(batch). Maybe a function in the utility, idk...
| condition = self.problem.conditions[condition_name] | ||
| pts = batch['pts'] | ||
| out = batch['output'] |
There was a problem hiding this comment.
same as PINN, I do not understand very well what is going on..
|
Overall the batching mechanism is very clever, I understood that the iterator in the dataloader returns two or three values, depending on whether data are there. I am concerned for a new user who wants to build a solver, the |
dario-coscia
added
pr-to-fix
|
Since the solvers are running, I would merge it now and open an issue in order to fix it once in 0.1 |
Ok, but let us fix the tests that are not running first, and check that GPU training is ok. |
| @@ -97,6 +97,15 @@ def configure_optimizers(self): | |||
| """ | |||
| return self.optimizers, [self.scheduler] | |||
|
|
|||
| def _loss_data(self, input, output): | |||
There was a problem hiding this comment.
also _loss_data and _loss_phys I would make it public. In such a way if the user wants to define a new PINN (e.g. gradient-PINN, weak-PINN, ...) he needs just to overwrite those functions.
There was a problem hiding this comment.
Private and public methods can be redefined in child classes!
Private methods/attributes just identify what shouldn't be called from external classes
* add dataset and dataloader for sample points * unittests
TODO list: